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.

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

Exported metric data has a count and sum but no min and max value #594

Closed
dayaffe opened this issue Aug 20, 2024 · 6 comments · Fixed by #601
Closed

Exported metric data has a count and sum but no min and max value #594

dayaffe opened this issue Aug 20, 2024 · 6 comments · Fixed by #601
Assignees

Comments

@dayaffe
Copy link

dayaffe commented Aug 20, 2024

I wanted to post this as a discussion since I don't know for sure whether it is my own user error or an opentelemetry-swift implementation issue.

I am currently using opentelemetry-swift along with ADOT to put metrics generated by using our SDK into AWS Cloudwatch. However, I noticed that when I switched the metric calculation from average to a percentile like p90 the value goes to 0. After inspecting the logs I discovered that my metrics are being sent as follows:

"smithy.client.duration": {
    "Max": 0,
    "Min": 0,
    "Count": 12,
    "Sum": 1.0121349096298218
},

Likely, cloudwatch is using min/max in its calculation of the p90 value which is why the data always goes to 0. Using a debugger I saw that min and max were there for each count of the datapoint (equal to whatever was in sum) but I would expect that across several counts, max != min != sum and max/min never to be 0.

Is there a way to fix this so that max and min are properly propagated to the exported data?

@dayaffe
Copy link
Author

dayaffe commented Aug 20, 2024

In https://github.com/open-telemetry/opentelemetry-swift/blob/f745a6435bad737dcb29807[…]porters/OpenTelemetryProtocolCommon/metric/MetricsAdapter.swift I see that min and max aren't added to .Histogram data point even though they are available.

@dayaffe
Copy link
Author

dayaffe commented Aug 20, 2024

This looks like a miss in the swift implementation of the library as other SDKs have addressed it such as open-telemetry/opentelemetry-js#3010 and is supposed to be required / default to true according to the specification https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#explicit-bucket-histogram-aggregation

@bryce-b
Copy link
Member

bryce-b commented Aug 20, 2024

Hey @dayaffe,
The metric implementation we have is a pretty old implementation. Take a look at the stable metrics, which will eventually replace the existing metrics in the SDK: https://github.com/open-telemetry/opentelemetry-swift/tree/main/Examples/Stable%20Metric%20Sample I believe this version calculates min/max properly

@dayaffe
Copy link
Author

dayaffe commented Aug 20, 2024

Hi @bryce-b, thanks for the reply! I believe I am using stable metrics. I actually used that link as an example but I still seem to hit the metrics/MetricsAdapter when using a debugger. Can you point me to where min/max is added in stable metrics so I can see if my code hits it correctly?

@dayaffe
Copy link
Author

dayaffe commented Aug 21, 2024

@bryce-b just did some more digging -- I am using StableOtlpMetricExporter as used in the example which in export calls MetricsAdapter class. The method I originally linked that doesnt add the min/max also expects Stable prefixed metric data toProtoMetric(stableMetric: StableMetricData).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants