Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add MeasurementProcessor specification to Metrics SDK #4318
base: main
Are you sure you want to change the base?
[WIP] Add MeasurementProcessor specification to Metrics SDK #4318
Changes from 2 commits
3f3186aa8f5de31ce9e4d4b0a58d449d2fb60adbd39d60b12File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we allow the processor to "drop" the measurement (e.g. the processor decided that it doesn't want the measurement) or other operations beyond modifications on the value and attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related question (thus decided to put it here).
Shouldn't the processor also be used when evaluating
Enabled?Shouldn't we also add an
OnEnabledhook?Related comment in other issue:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To allow processors to "drop" measurements, they must be somehow connected to the
MetricsReader. I agree that it would be a cool feature to have, providing great flexibility.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Lightstep Metrics SDK implements a MeasurementProcessor interface which was narrowly scoped to allow modifying the set of attributes for a measurement. In that use-case, we would take the incoming gRPC metadata from the context, look up specific headers, and apply header values as attribute values.
I admit I am not sure what reasons a user would have to modify measured values. Are there well-known use-cases? I found @jack-berg mentioned "unit conversion" here, but I am not sure how that would work--the measurement processor does not change the instrument definition, and the measurement does not include a unit. Are there really use-cases for modifying the value?
That SDK does not permit dropping measurements. Speaking also to @pellared's question about Enabled and whether measurement processors should intercept Enabled calls, I would recommend No. See my position on passing context to the metrics enabled method, #4256 (comment), which states the same. I am nervous about letting measurement processors change measurements and selectively enable/disable call sites because IMO it will make interpreting the resulting data very difficult.
As an example, suppose we have a measurement processor that is designed to redact sensitive attribute values. IMO it would be better to change attributes, not to drop events, because otherwise a user can be easily misled. Suppose we have a counter which counts requests with an attribute for success (boolean) and a client ID (string). We have a policy that says client IDs should not resemble e-mail addresses, otherwise they are invalid. The two options are to redact the client ID (e.g., give it a value like "redacted") or to drop the measurement. If we drop the measurement, all sorts of queries might be impacted. What's my success rate? I have no idea because an unknown number of redacted measurements were dropped.
Therefore, I would propose that measurement processors can only modify attributes, not values, and not drop events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing this feature without the ability to do unit conversion or drop measurements would be a miss. Can solve the lack of knowledge about unit by providing the processor access to instrument metadata. I think it could make sense to allow measurements processors to be configurable at the view level, in which case we might also consider allowing views to modify the unit of the resulting stream. Users could then compose a view which: 1. Adds a processor for unit conversion. 2. Adjusts the resulting stream's unit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll come around on this topic. I see how dropping metric events is a useful feature, despite the potential for difficult consequences. Dropping metric events is not very different than sampling traces at 0%. Just like 0% sampling (which we call "non probabilistic"), there is a loss of information, but that is intentional.
@jack-berg Given your statement, I think it means that the
Measurementtype should be defined as a 3-tuple (Value, Attributes, Instrument). This model works for me--and it resembles the OpenCensus "stats" API. Tangentially, I see a potential for us to form new APIs (like OpenCensus) which accept a list of measurements atomically and apply a single timestamp (e.g., or process the dynamic context once for multiple events).Let me pose a thought experiment. What does a MeasurementProcessor do better than you could achieve simply by wrapping a MeterProvider with a new instance containing the desired logic? I'm looking at the complexity trade-off here. I see how the desire to modify units comes about -- especially with the base-2 exponential histogram -- we see a desire to change seconds to/from milliseconds w/o loss of information as a compelling use-case. In the wrapped-MeterProvider scenario, the units-conversion wrapper would ("simply") register a new instrument with the delegate MeterProvider having different units and divide/multiply the value on its way through.
I thought of another case that I'm aware of, which calls for modifying the instrument kind, i.e., more than just a change of unit. I'm aware of use-cases for synchronous UpDownCounter instruments where the user would like to separate positive from negative values as two Counters. In this case, the two absolute value instruments convey the rate of ups and down as separate information. Still, the input-to-output mapping is 1:1.
I prefer to think of MeasurementProcessor as something like syntactic sugar for the example I described above, meaning that it can be defined abstractly as a wrapper of meter providers with a per-instrument event translation rule. There seems to be a potential -- do we know any use-cases? -- for one metric API event to translate into more than one metric API event on the wrapped meter provider. In this sense, we could define MeasurementProcessor as a per-instrument function that maps one input measurement into a list of zero or more output measurements, enabling both dropping and proliferation of events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmacd I think this makes sense. Having access to an
Instrumentinside the processor makes it very powerful.@jack-berg I'm reading the
Viewspecification, which explicitly mentions that views work on the "metric" level. Therefore, configuring processors on theViews (instead of onMeterProvider) would require updating theViewspecification as well, unless I'm misunderstanding something.Regarding dropping
Measurements, changing instrument kinds, modifying the value, or even creating newMeasurementson the fly (e.g., splitUpDownCounterinto two counters), we could make the proposedMeasure()method return an array ofMeasurementsinstead ofVoid.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if ForceFlush is required or even meaningful. I looked over our API and SDK specification, and I see no text explaining how a SDK should respond if there are metric events concurrent with flush and shutdown operations. Since I view MeasurementProcessor as a synchronous processor, all the same ambiguity applies. If a user is calling metric events while flushing or shutting down, the measurement processor may or may not have completed.
Since the processor is synchronous, it should have no buffered state to flush, it should just be synchronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, related to #4318 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on the simplification, removed these methods from the spec (60adbd3).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be tricky, what about MetricReader and the pull-based exporter? (e.g. Prometheus Exporter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, hence my comment. I referenced the
Exporterhere, but in the context of the Metrics SDK, it would only make sense to connectMeasurementProcessorandMetricReader.To make it work, the
MeasurementProcessorpipeline needs to exist between calls that record a new measurement, aka.Add()andRecord()APIs defined on theInstrument, but before the actual aggregation happens, as at that point, we lose reference toAttributes.So something along the lines of:
Instrument ---Record()/Add()---> MeasurementProcessor(s) ---> Views -> Aggregations -> MetricReaderHowever, I'm still trying to figure out if Views and Aggregations could somehow become built-in processors and whether it would even make sense.