Skip to content
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.

Sign up for GitHub

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
+48 −0
Draft

[WIP] Add MeasurementProcessor specification to Metrics SDK #4318

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f3186a
Add MeasurementProcessor specification to Metrics SDK
Blinkuu Dec 3, 2024
a8f5de3
Update TODO
Blinkuu Dec 3, 2024
1ce9e4d
Update specification/metrics/sdk.md
Blinkuu Dec 3, 2024
4b0a58d
Update specification/metrics/sdk.md
pellared Dec 3, 2024
449d2fb
Update specification/metrics/sdk.md
pellared Dec 3, 2024
60adbd3
Remove Shutdown and ForceFlush from MeasurementProcessor spec
Blinkuu Dec 6, 2024
9d60b12
Merge branch 'main' into add-measurement-processor-to-metrics-sdk-spec
Blinkuu Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions specification/metrics/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ linkTitle: SDK
* [Instrument advisory parameters](#instrument-advisory-parameters)
* [Instrument enabled](#instrument-enabled)
- [Attribute limits](#attribute-limits)
- [MeasurementProcessor](#measurementprocessor)
* [MeasurementProcessor operations](#measurementprocessor-operations)
+ [OnMeasure](#onmeasure)
- [Exemplar](#exemplar)
* [ExemplarFilter](#exemplarfilter)
+ [AlwaysOn](#alwayson)
Expand Down Expand Up @@ -984,6 +987,51 @@ Attributes which belong to Metrics are exempt from the
time. Attribute truncation or deletion could affect identity of metric time
series and the topic requires further analysis.

## MeasurementProcessor

Blinkuu marked this conversation as resolved.
Show resolved Hide resolved
**Status**: [Development](../document-status.md)

`MeasurementProcessor` is an interface which allows hooks when a `Measurement` is recorded by an `Instrument`.

`MeasurementProcessors` can be registered directly on SDK `MeterProvider` and they are invoked in the same order as they were registered.

SDK MUST allow users to implement and configure custom processors.

The following diagram shows `MeasurementProcessor`'s relationship to other components in the SDK:

```plaintext
+------------------+
| MeterProvider | +----------------------+ +-----------------+
| Meter A | Measurements... | | Measurements... | |
| Instrument X |-----------------> MeasurementProcessor +-----------------> In-memory state |
| Instrument Y + | | | |
| Meter B | +----------------------+ +-----------------+
| Instrument Z |
| ... | +----------------------+ +-----------------+
| ... | Measurements... | | Measurements... | |
| ... |-----------------> MeasurementProcessor +-----------------> In-memory state |
| ... | | | | |
| ... | +----------------------+ +-----------------+
+------------------+
```

### MeasurementProcessor operations

#### OnMeasure

`OnMeasure` is called when a `Measurement` is recorded. This method is called synchronously on the thread that emitted the `Measurement`, therefore it SHOULD NOT block or throw exceptions.

**Parameters:**

* `measurement` - a [Measurement](./api.md#measurement) that was recorded
* `context` - the resolved `Context` (the explicitly passed `Context` or the current `Context`)

**Returns:** Void

For a `MeasurementProcessor` registered directly on SDK `MeterProvider`, the `measurement` mutations MUST be visible in next registered processors.

Comment on lines +1031 to +1032
Copy link
Member

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?

Copy link
Member

@pellared pellared Dec 3, 2024 •
edited
Loading

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:

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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.

Copy link
Contributor

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.

Copy link
Author

@Blinkuu Blinkuu Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jmacd I think this makes sense. Having access to an Instrument inside the processor makes it very powerful.

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.

@jack-berg I'm reading the View specification, which explicitly mentions that views work on the "metric" level. Therefore, configuring processors on the Views (instead of on MeterProvider) would require updating the View specification as well, unless I'm misunderstanding something.

Regarding dropping Measurements, changing instrument kinds, modifying the value, or even creating new Measurements on the fly (e.g., split UpDownCounter into two counters), we could make the proposed Measure() method return an array of Measurements instead of Void.

A `MeasuremenetProcessor` may freely modify `measurement` for the duration of the `OnMeasure` call.

## Exemplar

**Status**: [Stable](../document-status.md)
Expand Down