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

WIP Add runtime/metrics source option to instrumentation/runtime metrics #2643

Closed
wants to merge 10 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 8, 2022

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.

instrumentation/runtime/doc.go Show resolved Hide resolved
instrumentation/runtime/doc.go Outdated Show resolved Hide resolved
@jmacd jmacd marked this pull request as ready for review August 26, 2022 22:38
@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #2643 (012c750) into main (bbbd638) will increase coverage by 0.0%.
The diff coverage is 80.6%.

Impacted file tree graph

@@          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     
Impacted Files Coverage Δ
instrumentation/runtime/builtin.go 77.9% <77.9%> (ø)
instrumentation/runtime/runtime.go 75.1% <100.0%> (+1.1%) ⬆️
samplers/jaegerremote/sampler_remote.go 85.4% <0.0%> (-2.0%) ⬇️

@jmacd jmacd marked this pull request as draft September 1, 2022 20:46
@jmacd
Copy link
Contributor Author

jmacd commented Sep 1, 2022

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.

@jmacd jmacd changed the title Add runtime/metrics source option to instrumentation/runtime metrics WIP Add runtime/metrics source option to instrumentation/runtime metrics Sep 8, 2022
@jmacd
Copy link
Contributor Author

jmacd commented Oct 13, 2022

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

@jmacd jmacd closed this Oct 13, 2022
This pull request was closed.
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 this pull request may close these issues.

Improvements to the instrumentation/runtime package
2 participants