-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[connector/servicegraph] Coalesce different attr sets into single Sco…
…peMetrics metric entry (#34070) Tests in `connector/servicegraph` were failing because the servicegraph `buildMetrics()` code was creating multiple metric entries for the same metric name within a single MetricScope. Although this may not be forbidden by the Otel specification, I think there is a general assumption that a metric name does not appear more than once within the same MetricScope. Instead, different values (e.g. with different sets of attribute values) should be created as separate datapoints within the same metric. The `pmetrictest.CompareScopeMetrics` test functionality is not designed to handle multiple metric entries with the same `Name()`. Instead, it is assumed that in cases where Order is ignored, the [first entry found in the actual metrics which matches the name of the expected metrics](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/ca9a8d14df471ed6a7dda7562ddfe659ec52127d/pkg/pdatatest/pmetrictest/metrics.go#L186-L195) *must* be the metric to compare. This fix changes the `buildMetrics()` code to create one metric within a scope, and instead create multiple datapoints per metric when there are entries where the datapoint attribute set is unique (i.e. all entries in the internal maps `serviceGraphConnector.req{Total,FailedTotal,ServerDurationSecondsCount}` are coalesced into a single named metric as appropriate.) _Note_: I don't have any past experience working with servicegraphconnector, but just observing that `collectClientLatencyMetrics()` and `collectServerLatencyMetrics()` both range over the same map - `p.reqServerDurationSecondsCount`, although the actual values collected in `collectClientLatencyMetrics()` are from `p.reqServerDurationSeconds{Count,Sum,BucketCounts}` and the values collected in `collectServerLatencyMetrics()` are from `p.reqClientDurationSeconds{Count,Sum,BucketCounts}`. This seems a little asymmetrical, but I don't have enough experience to say whether this is an error or not. **Description:** Fixes #33998 **Testing:** All other unit tests now complete, and the previously failing unit test now works reliably. **Documentation:** No documentation added. This is a unit test fix.
- Loading branch information
1 parent
7e3fc98
commit f2cfc2d
Showing
3 changed files
with
73 additions
and
80 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters