Update runtime instrumentation to new semantic conventions

open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.

https://opentelemetry.io/

Apache License 2.0

1.15k stars 544 forks source link

Update runtime instrumentation to new semantic conventions #5655

Open MrAlias opened 4 months ago

MrAlias commented 4 months ago

https://github.com/open-telemetry/semantic-conventions/pull/981

dashpole commented 4 months ago

Do we want a migration plan? Or do we want to just want to switch to the new conventions?

MrAlias commented 4 months ago

+1 to just switching.

MrAlias commented 4 months ago

Migration plan:

Add the new semantic conventions, these will be used by default
Add option to turn on old metrics (in addition to the new which are default)

The old metrics will be supported for 2 releases. We do not plan to make any changes to the old metrics. If there is any CVEs they will be removed prior to the 2 release timeline.

dashpole commented 2 months ago

I just need to implement the histogram metric now.

dashpole commented 2 months ago

Some notes from my rough attempt to implement the histogram metric:

It has a lot of buckets (160?). See the source: https://github.com/golang/go/blob/355711821eea51c6456a31ab61d0dc2e9db034f7/src/runtime/histogram.go#L71
It seems like the buckets may map to our exponential histogram. We might want to consider using that instead of a fixed-bucket histogram.
It doesn't have a Sum. I'm not sure if we can reasonably GA it without a sum. We may need to ask the Go team to add the Sum.

MrAlias commented 2 months ago

It seems like the buckets may map to our exponential histogram. We might want to consider using that instead of a fixed-bucket histogram.

"buckets are power-of-2 sized" seems to motivate this :+1:

MrAlias commented 2 months ago

It doesn't have a Sum. I'm not sure if we can reasonably GA it without a sum. We may need to ask the Go team to add the Sum.

Sounds like a good ask for the Go team

dashpole commented 2 months ago

"buckets are power-of-2 sized" seems to motivate this 👍

It is powers-of-2 sized, but with linear "sub-buckets" between the power-of-two buckets. I asked, and the buckets are also subject to change over time, so using an exponential histogram seems like not the best idea. Slightly disappointing, but maybe an opportunity to improve in the future.

dashpole commented 1 month ago

I'm going to track adding the histogram metrics separately: https://github.com/open-telemetry/opentelemetry-go-contrib/issues/5974.