-
Notifications
You must be signed in to change notification settings - Fork 589
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
WIP Add runtime/metrics source option to instrumentation/runtime metrics #2643
Conversation
… jmacd/runtimemetrics
Codecov Report
@@ Coverage Diff @@
## main #2643 +/- ##
======================================
Coverage 74.4% 74.5%
======================================
Files 145 146 +1
Lines 6681 6828 +147
======================================
+ Hits 4973 5087 +114
- Misses 1561 1591 +30
- Partials 147 150 +3
|
I feel this deserves more attention, and I don't feel the group has sufficient bandwidth while the metrics Alpha is still under development. I will bring this up for discussion, meanwhile will be testing a new package copied from this PR in the lightstep/otel-launcher-go repo. |
Lightstep will work to submit their downstream fork of this code to the upstream repository. For now, see https://github.com/lightstep/otel-launcher-go/tree/main/lightstep/instrumentation |
Fixes #2624
The code in
instrumentation/metrics
was contributed a long time ago. The Go team has since introduced runtime metrics in an official way. This PR adds optional support for the new metrics. There is a problem in the old implementation because it was written before the current SDK specification; it is no longer correct to use synchronous instruments from async callbacks--where there are more than one reader concerned. This leaves the old code as-is and focuses on the desired future outcome without changing current default behavior.I propose that we deprecate the old and adopt the new eventually, however there are a number of gotchas.
There are four histogram instruments, but we lack a way to record them asynchronously. Moreover, one is a Gauge Histogram which OTel has not specified.
open-telemetry/opentelemetry-specification#2713
open-telemetry/opentelemetry-specification#2714
There are questions about what is a UpDownCounter and what is a Gauge. They are all currently UpDownCounters, but that is no guarantee. Presently, if a non-cumulative metric is Uint64, it becomes UpDownCounter, and if the value is Float64 it becomes a Gauge. We have no way to test this Gauge path presently because the go-1.19 runtime currently produces all Uint64 metrics and Float64Hist metrics, no Float64 metrics.
Lastly, there are two cases in 1.19 where there are duplicate metric names, after removing units. This creates a technical problem for OTLP -> Prometheus -> OTLP; currently, the only way to do this that satisfies both groups' specifications is to use an empty unit string when the unit is in the metric name itself. Thus, when there is a conflict of this nature, the units are kept in the name and the OTel unit is empty. This is an area for development between OpenTelemetry and OpenMetrics/Prometheus.