-
Notifications
You must be signed in to change notification settings - Fork 850
[OTLP] Avoid redundant allocations and log HTTP export success #6562
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
[OTLP] Avoid redundant allocations and log HTTP export success #6562
Conversation
Add entries for open-telemetry#6562.
Avoid allocations when logging in OTLP export clients.
Log successful HTTP export like we do for gRPC.
Add entries for open-telemetry#6562.
8d6af25 to
f264179
Compare
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.
Pull Request Overview
This PR optimizes OTLP export clients by avoiding redundant string allocations in logging and adds parity between HTTP and gRPC export clients by logging successful HTTP exports.
- Avoids
Uri.ToString()allocations when event source is not enabled at the relevant logging levels - Adds success logging for HTTP export to match existing gRPC behavior
- Introduces helper methods with logging level checks to prevent unnecessary allocations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| OpenTelemetryProtocolExporterEventSource.cs | Added helper methods with event level checks to avoid Uri.ToString() allocations |
| OtlpHttpExportClient.cs | Added success logging for HTTP export completion |
| OtlpGrpcExportClient.cs | Updated to use new helper methods that avoid string allocations |
| CHANGELOG.md | Updated to document performance improvements and feature parity |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6562 +/- ##
==========================================
- Coverage 86.63% 86.62% -0.01%
==========================================
Files 258 258
Lines 11888 11895 +7
==========================================
+ Hits 10299 10304 +5
- Misses 1589 1591 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
|
||
| ## Unreleased | ||
|
|
||
| * Avoid allocating strings when logging gRPC export events unless required. |
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.
No need to update changelog for this change as it is related to internal logging.
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.
left a comment to remove updates to changelog
|
|
||
| ## Unreleased | ||
|
|
||
| * Log export success when exporting telemetry using `http/protobuf`. |
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.
We don't need to include this update in the changelog, as it doesn't have any relevance for the customer.
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.
Is there no way for users to enable it by listening the event source themselves if they want to debug?
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.
Including these kinds of updates in the changelog could quickly become too noisy. Since this isn’t something most customers will act on directly, I’d lean toward leaving it out and this is what earlier maintainers too followed as guidelines in this repo.
Changes
Found while working on #6463.
Uri.ToString()allocations when logging in OTLP export clients if the event source is not enabled at the relevant logging levels.Merge requirement checklist
Unit tests added/updatedCHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)