[receiver/googlecloudmonitoring: Support distribution metric #40216
Conversation
… type format Signed-off-by: Joffref <[email protected]>
Joffref
changed the title
|
Please add tests and a changelog. |
|
@atoulme I'm on my way to add the changelog and fix the linting errors. I don't see where I should add tests—could you guide me? I only see some tests related to receiver creation. |
Signed-off-by: Joffref <[email protected]>
|
at minimum a set of unit tests calling |
|
|
||
| // Handle bucket options based on type | ||
| if bucketOpts := distValue.BucketOptions; bucketOpts != nil { | ||
| if expBuckets := bucketOpts.GetExponentialBuckets(); expBuckets != nil { |
There was a problem hiding this comment.
Question: This is not doing any translation for bucket boundaries/not translating to an OTel exponential histogram. How does this work?
There was a problem hiding this comment.
I think I got confused here between the GCM data model and the OTEL one — I’ll take a deeper look. Moving this PR to draft, and happy to incorporate your additions!
|
@Joffref happy to collaborate on this one. I have been wanting to finish up a similar PR to make exponential GCM distributions work. Can share parts of that tomorrow/Saturday most likely. |
|
Hey @bripkens! Sure, happy to have your help—I'm still quite new to contributing to OTEL. I'll address your comment later. |
| if point.Interval.EndTime != nil && point.Interval.EndTime.IsValid() { | ||
| dp.SetTimestamp(pcommon.NewTimestampFromTime(point.Interval.EndTime.AsTime())) | ||
| } else { | ||
| mb.logger.Warn("EndTime is invalid for metric:", zap.String("Metric", ts.GetMetric().GetType())) |
There was a problem hiding this comment.
Should we set the end time to the current time in this case?
|
|
||
| distValue := point.Value.GetDistributionValue() | ||
| if distValue == nil { | ||
| mb.logger.Warn("Distribution value is nil for metric:", zap.String("Metric", ts.GetMetric().GetType())) |
There was a problem hiding this comment.
Should we do this check before we AppendEmpty to the datapoints so that we don't pass through empty distribution points?
| dp.SetCount(uint64(distValue.Count)) | ||
|
|
||
| // Handle bucket options based on type | ||
| if bucketOpts := distValue.BucketOptions; bucketOpts != nil { |
There was a problem hiding this comment.
It will probably be helpful for you to look at the exporter implementation:
Exponential Histograms: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/33641345ef4756e48d7011fbb38a5c64ec6b3827/exporter/collector/metrics.go#L1071
| // Set exemplars | ||
| if exemplars := distValue.Exemplars; exemplars != nil { | ||
| for _, exemplar := range exemplars { | ||
| dp.Exemplars().AppendEmpty().SetDoubleValue(float64(exemplar.Value)) |
There was a problem hiding this comment.
You will want to make sure you get the trace ID and span ID from the SpanContext exemplar attachment. You will also want to set the exemplar's filtered attributes from the DroppedLabels attachment. See https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/33641345ef4756e48d7011fbb38a5c64ec6b3827/exporter/collector/metrics.go#L967
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…40688) #### Description This adds support for converting Google Cloud monitoring delta distribution metrics to OpenTelemetry histograms. #### Link to tracking issue This is related to #39600, although it might not be a complete solution, since this only contributes delta distributions, but not cumulative distributions. #### Testing This has been tested extensively with the metric `run.googleapis.com/request_latencies`. Here is a comparison of the p95 of that metric directly in Google Cloud and of the OTel metric as produced by the new code in a third party tool (Dash0). <img width="1742" alt="20250612_request_latencies_comparison" src="https://github.com/user-attachments/assets/dd88d338-3abd-4dfb-a969-72af7c345a5c" /> There charts have minor differences which I suspect are introduced by different interpolation strategies in the two tools (we only have one datapoint per minute), but besides that, the charts clearly show the same data -- peaks and valleys are at the same timestamps and the y-axis values also match. There are also comprehensive unit tests for the new functionality in `receiver/googlecloudmonitoringreceiver/internal/metrics_conversion_test.go`. #### Remarks Unfortunately I only saw #40216 after I had more or less finished my implementation here. I realize that this is not ideal, since there are now two different PRs that attempt to add the same feature. I am not sure if the author of #40216 is still working on their PR actively. I think the last status there seems to be that tests are missing. I'm happy to port over changes from #40216 in case it has things that are missing here or help porting over changes from here to #40216, if that helps. That said, this PR has a full set of unit tests and it also has support for converting the span context exemplar (although that certainly needs to be double checked, see inline comments). --------- Co-authored-by: Ben Blackmore <[email protected]>
|
Duplicate of #40688, great job @basti1302! |
Description
Support for the
DISTRIBUTIONmetric kind. Learn more: https://cloud.google.com/monitoring/api/v3/kinds-and-types#metric-kindsLink to tracking issue
Fixes #39600
Testing
I've tested this patch with the following metrics:
run.googleapis.com/container/cpu/utilizationsrun.googleapis.com/container/memory/utilizationsIt likely needs broader testing—I'm happy to try it out with more relevant Google metrics.