open-telemetry / opentelemetry-swift

OpenTelemetry API for Swift
https://opentelemetry.io/docs/instrumentation/swift/
Apache License 2.0
221 stars 145 forks source link

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

Closed dayaffe closed 1 month ago

dayaffe commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

This looks like a miss in the swift implementation of the library as other SDKs have addressed it such as https://github.com/open-telemetry/opentelemetry-js/issues/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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).

bryce-b commented 2 months ago

It looks like min/max is missing from here: https://github.com/open-telemetry/opentelemetry-swift/blob/main/Sources/Exporters/OpenTelemetryProtocolCommon/metric/MetricsAdapter.swift#L146C8-L153C64