-
Notifications
You must be signed in to change notification settings - Fork 354
[Exporter.Geneva] Add httpUrl for HTTP server spans #2818
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
[Exporter.Geneva] Add httpUrl for HTTP server spans #2818
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #2818 +/- ##
===========================================
- Coverage 73.91% 53.93% -19.98%
===========================================
Files 267 55 -212
Lines 9615 5141 -4474
===========================================
- Hits 7107 2773 -4334
+ Misses 2508 2368 -140
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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 adds support for composing an httpUrl for HTTP server spans by combining multiple HTTP semantic convention attributes. Key changes include adding unit tests for HTTP URL composition in both unstable and stable semantic convention scenarios, updating Geneva exporter tests, and extending the MsgPackTraceExporter to cache and build the HTTP URL based on new mappings.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/OpenTelemetry.Exporter.Geneva.Tests/MsgPackTraceExporterTests.cs | Tests validating httpUrl parts caching and URL generation |
| test/OpenTelemetry.Exporter.Geneva.Tests/GenevaTraceExporterTests.cs | Integration tests for verifying HTTP URL mapping in server and client spans |
| src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs | Updates to caching mechanisms and HTTP URL composition logic using new mapping dictionaries |
| src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md | Changelog updated with new HTTP URL mapping feature |
Comments suppressed due to low confidence (2)
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs:489
- [nitpick] Consider renaming the variable 'httpMethodString' (assigned at this point) to 'httpMethodTagKey' to more explicitly indicate that it holds the tag key rather than the HTTP method value.
if (isServerActivity && replacementKey == "httpMethod")
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs:462
- [nitpick] Consider adding an inline comment here to clarify the initialization and reset behavior of the 'HttpUrlParts' thread-local variable to improve code maintainability.
var httpUrlParts = this.HttpUrlParts.Value ?? new object?[CS40_PART_B_HTTPURL_MAPPING.Count];
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs
Outdated
Show resolved
Hide resolved
…validity in GetHttpUrl method
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, there is a minor style error please fix it.
Fixes #
Design discussion issue #
Changes
Please provide a brief description of the changes here.
HTTP semconv: Combination of
url.scheme,server.address,server.port,url.pathandurl.queryattributes for HTTP server spans.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes