Implement PoC of MeasurementProcessor proposal #4642
Conversation
|
|
||
| ```python | ||
| processor = StaticAttributeMeasurementProcessor({ | ||
| "environment": "production", |
There was a problem hiding this comment.
I am not sure if this is a good idea. For StaticAttributes for all measurements, couldn't we model it as Resource Attributes. If applicable to a subset, then maybe use Meter attributes?
There was a problem hiding this comment.
We could - this is just an example.
IMO, none of these implementations I'm showing should be present in opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_processor.py. I just placed them there since this is a PoC for simplicity's sake.
When we merge open-telemetry/opentelemetry-specification#4318 and start working on the actual implementation, the concrete processors should probably be contributed here: https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/processor
|
|
||
| #### 4. ValueRangeMeasurementProcessor | ||
|
|
||
| Drops measurements outside a specified value range. |
There was a problem hiding this comment.
while this looks valid, I am not sure if its common to drop measurements based on value....
May I suggest another one which seems to me like a very valid use case - collapsing certain attributes to lower the cardinality. Described here: open-telemetry/opentelemetry-dotnet#6332
There was a problem hiding this comment.
Removed ValueRangeMeasurementProcessor completely. Meters seem to handle invalid values themselves.
| ) -> None: | ||
| # Example: Add timestamp attribute | ||
| new_attributes = dict(measurement.attributes or {}) | ||
| new_attributes["processed_at"] = str(int(time.time())) |
There was a problem hiding this comment.
I don't think we should show example of adding timestamp as a Metric attribute - is there any valid use-case for it?
| next_processor(modified_measurement) | ||
|
|
||
| # Unit conversion processor | ||
| class MetersToFeetProcessor(MeasurementProcessor): |
There was a problem hiding this comment.
I like this! A simpler one would be to do sec->msec.
Qn : Should a view also be added to change the unit to account for this processor?
There was a problem hiding this comment.
Qn : Should a view also be added to change the unit to account for this processor?
Umm, I'm not sure. Perhaps someone with more experience with Metrics spec could answer this question.
| else: | ||
| next_processor(measurement) | ||
|
|
||
| # Sampling processor |
There was a problem hiding this comment.
not sure of the utility of this for Metrics!
| try: | ||
| request_counter.add(1, {"endpoint": "/api/users", "method": "GET"}) | ||
| response_time_histogram.record( | ||
| 0.150, {"endpoint": "/api/users", "status": "200"} |
There was a problem hiding this comment.
nit: if status_code, should it be 200 and not "200"?
Description
This PR is a Proof of Concept of the MeasurementProcessor concept for the OpenTelemetry Python SDK, following the design specified in OpenTelemetry Specification PR #4318.
MeasurementProcessor enables powerful measurement processing capabilities including:
The implementation uses a chain-of-responsibility pattern where each processor decides whether to pass, modify, or drop measurements by calling (or not calling) the next processor in the chain.
Key components added:
MeasurementProcessorabstract interfaceMeasurementProcessorChainfor managing processor executionMeterProvider,SdkConfiguration, andMeasurementConsumerUsage:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I'm not a Python expert, and this is just a PoC. Therefore, no extensive testing has been conducted.
There are some tests that can be run with the following bash command:
You can also see and run the example:
Does This PR Require a Contrib Repo Change?
Checklist: