Update Instrumentation to metrics v0.28.0 #1977
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MadVikingGod
requested review from
jmacd,
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
paivagustavo,
pellared and
hanyuancheung
as code owners
MadVikingGod
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1977 +/- ##
=======================================
+ Coverage 72.1% 72.3% +0.1%
=======================================
Files 124 126 +2
Lines 5330 5412 +82
=======================================
+ Hits 3848 3916 +68
- Misses 1370 1379 +9
- Partials 112 117 +5
|
MrAlias
approved these changes
Mar 24, 2022
Aneurysm9
approved these changes
Mar 24, 2022
| return err | ||
| } | ||
|
|
||
| err = r.meter.RegisterCallback( |
There was a problem hiding this comment.
Should this be three separate callbacks as it was before? I'm not sure what impact combining them has.
There was a problem hiding this comment.
I don't believe that any of the callbacks are run in separate routines. So this would just dispense with the overhead of tracking 3 different closures and calling each one serially.
dashpole
reviewed
Mar 24, 2022
dashpole
approved these changes
Mar 24, 2022
MadVikingGod
commented
Mar 24, 2022
instrumentation/github.com/astaxie/beego/otelbeego/test/beego_test.go
Outdated
Show resolved
Hide resolved
instrumentation/github.com/astaxie/beego/otelbeego/test/beego_test.go
Outdated
Show resolved
Hide resolved
instrumentation/github.com/astaxie/beego/otelbeego/test/beego_test.go
Outdated
Show resolved
Hide resolved
| return err | ||
| } | ||
|
|
||
| err = r.meter.RegisterCallback( |
There was a problem hiding this comment.
I don't believe that any of the callbacks are run in separate routines. So this would just dispense with the overhead of tracking 3 different closures and calling each one serially.
Co-authored-by: Tyler Yahn <[email protected]> Co-authored-by: David Ashpole <[email protected]>
|
In an effort to enable users to test the new release I will merge this sooner than the 24h window. This already has approvals from the maintainers and 1+ approver. |
Merged
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Mar 8, 2023
…x` is "unstable", and as it specifieds a minimum version, that will include "any breaking change after".
try to fix go.opentelemetry.io dependency hell
docker ("indirect") and buildkit ("direct") specify `v0.29.0`;
```bash
go mod graph --modfile=vendor.mod | grep ' go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp'
github.com/docker/docker go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
github.com/moby/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
```
containerd specified v0.34.0 for `go.opentelemetry.io/otel/metric`;
```bash
go mod graph --modfile=vendor.mod | grep ' go.opentelemetry.io/otel/metric@'
github.com/docker/docker go.opentelemetry.io/otel/[email protected]
github.com/containerd/[email protected] go.opentelemetry.io/otel/[email protected]
github.com/moby/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/internal/[email protected] go.opentelemetry.io/otel/[email protected]
```
But BuildKit uses;
```bash
git checkout v0.11.4
go mod graph | grep ' go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp'
github.com/moby/buildkit go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
k8s.io/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
k8s.io/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
```
But looks like BuildKit has older versions of the other packages installed;
```bash
go mod graph | grep ' go.opentelemetry.io/otel/metric@'
github.com/moby/buildkit go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/internal/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/exporters/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/sdk/export/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/sdk/[email protected] go.opentelemetry.io/otel/[email protected]
```
Looks like `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.31.0` is the first version taking the new metrics into account;
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/instrumentation/net/http/otelhttp/v0.31.0/instrumentation/net/http/otelhttp/handler.go#L52-L53
Changed in open-telemetry/opentelemetry-go-contrib@a587520 (open-telemetry/opentelemetry-go-contrib#1977)
Unfortunately, updating these to v0.31.0 works around the other issues, but breaks BuildKit, which doesn't support the newer API;
go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace v0.31.0 // indirect // updated to v0.31+ to account for moby#44530 (comment)
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.31.0 // indirect // updated v0.31+ to account for moby#44530 (comment)
```bash
Building static bundles/binary-daemon/dockerd (linux/arm64)...
vendor/github.com/moby/buildkit/util/tracing/transform/span.go:34:26: sd.InstrumentationLibrarySpans undefined (type *"github.com/docker/docker/vendor/go.opentelemetry.io/proto/otlp/trace/v1".ResourceSpans has no field or method InstrumentationLibrarySpans)
vendor/github.com/moby/buildkit/util/tracing/transform/span.go:56:17: undefined: v11.InstrumentationLibrary
vendor/github.com/moby/buildkit/util/tracing/transform/instrumentation.go:9:42: undefined: commonpb.InstrumentationLibrary
vendor/github.com/moby/buildkit/worker/containerd/containerd.go:56:74: cannot use &gogoptypes.Empty{} (value of type *"github.com/docker/docker/vendor/github.com/gogo/protobuf/types".Empty) as *emptypb.Empty value in argument to client.IntrospectionService().Server
```
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following Instrumentation packages use the metrics API, and are updated to v0.28.0
Note:
In v0.27.0 of the metrics API there was otel/metric/metrictest. This had a MeterProvider that did not use the SDK to record metrics in memory. That was removed from v0.28.0 and will be replaced in open-telemetry/opentelemetry-go#2722