-
Notifications
You must be signed in to change notification settings - Fork 304
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
[geneva] Add support for exporting otlp metrics via user_events on Linux #2113
[geneva] Add support for exporting otlp metrics via user_events on Linux #2113
Conversation
[user_events](https://docs.kernel.org/trace/user_events.html) on Linux when | ||
OTLP protobuf encoding is enabled via the | ||
`PrivatePreviewEnableOtlpProtobufEncoding=true` connection string switch. | ||
([#2113](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2113)) |
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.
With this, PrivatePreviewEnableOtlpProtobufEncoding=true
is now supported in both Widows and Linux. Windows uses ETW as transport, while Linux uses user-events as transport.
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.
@CodeBlanch can you add this to changelog before merge
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.
Updated CHANGELOG and REDME where this stuff was mentioned
src/OpenTelemetry.Exporter.Geneva/Metrics/OtlpProtobuf/OtlpProtobufMetricExporter.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Metrics/Transport/MetricUnixUserEventsDataTransport.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Metrics/Transport/MetricUnixUserEventsDataTransport.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Metrics/Transport/MetricUnixUserEventsDataTransport.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Metrics/Transport/MetricUnixUserEventsDataTransport.cs
Show resolved
Hide resolved
It is a huge PR with lots of changes, some are related to the title or the PR, some are clean ups and renames, etc. Could you @CodeBlanch break it up to smaller PRs? Thanks! |
test/OpenTelemetry.Exporter.Geneva.Tests/UnixUserEventsDataTransportTests.cs
Show resolved
Hide resolved
@cijothomas This should be good for a final review. What I was doing was working to get something up and running on an actual distro to validate everything: https://github.com/CodeBlanchOrg/opentelemetry-dotnet-contrib/actions/runs/11249188144/job/31276142590?pr=53#step:7:158 Tested now using WSL2 & ubuntu-22.04 |
@@ -73,7 +73,7 @@ private bool Connect() | |||
} | |||
catch (Exception ex) | |||
{ | |||
ExporterEventSource.Log.ExporterException("UDS Connect failed.", ex); | |||
ExporterEventSource.Log.TransportException(nameof(UnixDomainSocketDataTransport), "Connection failure", ex); |
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.
nit: Include something like "will retry connecting in background", to indicate this may be recovered.
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.
Updated
public class UnixUserEventsDataTransportTests | ||
{ | ||
/* | ||
* Instructions for running these tests: |
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.
Thanks for writing this steps!
My own linux kernel with user-events is broken, so not testing it myself, but will comeback to this as soon as kernel is updated in wsl!
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.
THe main readme on genevaexporter can now say "exports to ETW, UnixSocket or user events"!
@reyang @rajkumar-rangaraj Would be good to get one more pair of eyes before merge. |
FYI - most likely I won't have bandwidth in the next 4-5 weeks. |
|
||
ExporterEventSource.Log.TransportInformation( | ||
nameof(MetricUnixUserEventsDataTransport), | ||
$"Tracepoint registration for 'otlp_metrics' failed with recoverable result: '{this.metricsTracepoint.RegisterResult}'. Entering running state."); |
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.
This message is slightly confusing, we are saying failed with recoverable result and entering running state too.
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.
Tweaked
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, I like the way PerfTracepointListener
is implemented. :)
Changes
Details
Users have to opt-into this via connection string:
TODOs
Planning to do this stuff as follow-up PRs:
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes