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
Add MeasurementProcessor specification to Metrics SDK #4318
base: main
Are you sure you want to change the base?
Add MeasurementProcessor specification to Metrics SDK #4318
Changes from 3 commits
3f3186a
a8f5de3
1ce9e4d
4b0a58d
449d2fb
60adbd3
9d60b12
17f650c
4cbc0ec
01bcc0a
d28e993
90d331d
ee72e3f
225000a
8fbc341
f57fe00
File 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.
In the context of tracing, a processor is somewhat of a different thing (it's not a filterer of data, it sits on top of exporters).
Should we name this differently than
Processor
to differenciate the two?Or should we make both behaviors similar?
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
MeasurementProcessor
is more than a filter of data. It can modify the measurement, decide to drop it, or even emit it multiple times.To me, they are semantically similar. The difference is in the architecture of the Metrics SDK, which makes it hard to fit and connect the new processor pipeline directly with exporters. We could revisit this in the future and try to align the behavior between Logs, Traces, and Metrics SDKs. To keep an open door for such ideas, I'm purposefully specifying that:
Built-in measurement processors are responsible for Measurement Processing.
Where
Measurement Processing
is currently vaguely defined in this spec by using the wordSHOULD
.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 support @Blinkuu's reasoning above. In my thinking, we can use "processor" across OpenTelemetry when a component in a program receives and outputs the same type of data. Trace SDK processors input/output span data. Collector processors input/output pipeline data. Things that are not processors (e.g., Metric reader, Trace sampler, Exporters, Receivers) generally have different types of input and output.
Here, MeasurementProcessor is appropriate because the input and the output types are the same.
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
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.
Unclear how is this part works with the later wording "The SDK MUST inject the
OnMeasure
function from the next processor in the chain into the current processor"Who is invoking the MeasurementProcessors in the order? Is that MeterProvider/SDK ? Or is it the responsibility of each
MeasurementProcessor
to invoke the next one in the chain?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 have changed the wording around the
next
parameter, as the previous spec was confusing. Thenext
parameter is a callback that MUST NOT require a reference to the next processor in the chain. On the implementation level, it will most likely end up as a closure. I see it as the SDK's responsibility to ensure processors are executed in the order they were configured on theMeterProvider
.That being said, every processor is able to decide whether it wants to invoke a chain of processors that follow it (by calling
next(context, measurement)
). Each processor can decide to callnext
either0
(drop measurement),1
(default flow), orN
(custom use cases) times.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
OnEnabled
hook?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
Measurement
type 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
Instrument
inside the processor makes it very powerful.@jack-berg I'm reading the
View
specification, which explicitly mentions that views work on the "metric" level. Therefore, configuring processors on theView
s (instead of onMeterProvider
) would require updating theView
specification as well, unless I'm misunderstanding something.Regarding dropping
Measurements
, changing instrument kinds, modifying the value, or even creating newMeasurements
on the fly (e.g., splitUpDownCounter
into two counters), we could make the proposedMeasure()
method return an array ofMeasurements
instead 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.
@Blinkuu About view-attached processors: I am generally wary of making the metrics SDK more complex, and the idea of making measurement processors view-specific has me vaguely worried. One fear is that this will limit the potential for MeasurementProcessors to be optimized.
If there are multiple readers and multiple measurement processors, do we evaluate the chain of processors per reader or once? I would prefer once.
I fear we're letting implementation details into the specification, if we dictate the use of a "next" processor here. I don't think a next processor is required. A better API for the processor IMO would be to return a measurement, so a signature like
In other words, the return value is an optional Measurement. I suggest the specification use pseudo-code such as the following, to answer the question raised in https://github.com/open-telemetry/opentelemetry-specification/pull/4318/files#r1915514480 about the order of operations, and this makes the drop-behavior clear:
If there's an argument in favor of view-specific measurement processors, I would suggest surveying the implementors of the various metrics SDKs view mechanisms for their opinion. Otherwise, I think it's much simpler to explain to users what's happening with measurement processors: they literally change the events that enter the SDK and are seen by all metric readers alike.
@Blinkuu regarding an array of measurements. This is another question that could impact performance. Unless we need it, I don't think one measurement should be allowed to become >1 measurement, in other words. In the example I gave of translating an UpDownCounter into two Counters, one input event becomes one output event.