-
Notifications
You must be signed in to change notification settings - Fork 110
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
Upgrade to OTel v0.116.0 #1484
Upgrade to OTel v0.116.0 #1484
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1484 +/- ##
==========================================
+ Coverage 81.34% 81.37% +0.03%
==========================================
Files 149 149
Lines 15264 15268 +4
==========================================
+ Hits 12416 12425 +9
+ Misses 2244 2240 -4
+ Partials 604 603 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I tried to run just |
@grafana/beyla Is there any chance that you could remove this go mod replace from your go.mod file please:
It is causing an issue with the Alloy build:
|
Thanks @ptodev, I'll submit a PR with the license change for you. Regarding the replace in the go.mod, we still need this to support removal of OTel metrics, since we haven't yet merged the change upstream. Depending on how urgent this is, we can temporarily remove it and re-instate it after Alloy picks up the version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Looks good, I just had one question about the removal of the levelled metrics config option.
@@ -376,11 +375,8 @@ func getTraceSettings(ctxInfo *global.ContextInfo, in trace.SpanExporter) export | |||
} | |||
meterProvider := metric.NewMeterProvider() | |||
telemetrySettings := component.TelemetrySettings{ | |||
Logger: zap.NewNop(), | |||
MeterProvider: meterProvider, | |||
LeveledMeterProvider: func(_ configtelemetry.Level) metric2.MeterProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind providing a bit of detail why we don't need this anymore? I think this was ported over from Alloy to Beyla by @marctc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think probably in newer versions of OTEL LeveledMeterProvider
is no needed anymore, so I guess it's all good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @ptodev, thanks for this!
I believe this is required in order to update Alloy to OTel v0.116.0 as well.
I was not able to run
make
on my Mac due to this error:I suppose the build only works on Linux? I hope the PR CI will let me know if anything else such as
third_party_licenses.csv
needs updating.