-
Notifications
You must be signed in to change notification settings - Fork 273
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
[Instrumentation.AspNet] pass http.request.method to sampler #2023
base: main
Are you sure you want to change the base?
Conversation
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/CHANGELOG.md
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/CHANGELOG.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/CHANGELOG.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
{ | ||
tags = cachedTagsStorage ??= new KeyValuePair<string, object?>[1]; | ||
string? path = context.Request?.Unvalidated?.Path; | ||
string? method = context.Request?.HttpMethod; |
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 we need to set the normalized value as we do here
If the method is normalized to _OTHER
then should we also include http.request.method_original, that part is not very clear in spec
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.
fixed 34e7fa3
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 setting "HTTP" like the example you sent is the way to go. Keeping the convention.
src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs
Outdated
Show resolved
Hide resolved
@cijothomas @Kielek this would be really useful for us.... can we have it available sooner |
@@ -64,22 +65,38 @@ public static bool HasStarted(HttpContext context, out Activity? aspNetActivity) | |||
PropagationContext propagationContext = textMapPropagator.Extract(default, context.Request, HttpRequestHeaderValuesGetter); | |||
|
|||
KeyValuePair<string, object?>[]? tags; |
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.
Why not replace KeyValuePair<string, object?>[]?
with List<KeyValuePair<string, object?>>?
for dynamic tag management, which eliminates the need for manual index handling in arrays, making the code more flexible and cleaner?
Fixes #57287
Changes
Added http.request.method to tags given to sampler
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes