Skip to content
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

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
961e5be
add http.method to create activity
roycald245 Aug 15, 2024
993d7bc
test
roycald245 Aug 15, 2024
29fbce7
remove comment
roycald245 Aug 26, 2024
9078fe8
Merge branch 'main' into feat/add-http-method
roycald245 Aug 26, 2024
353d9d6
edit changelog
roycald245 Aug 26, 2024
d6d0942
edit readme
roycald245 Aug 26, 2024
dce0aac
update tests
roycald245 Aug 26, 2024
d2e511e
lint
roycald245 Aug 26, 2024
8855fd1
lint markdown
roycald245 Aug 26, 2024
f62e71c
fix passing url.path to http.request.method
roycald245 Aug 26, 2024
4c3c59a
Edit changelog
roycald245 Aug 26, 2024
114cabb
rephrase changelog
roycald245 Aug 26, 2024
8494e65
Update CHANGELOG.md
roycald245 Aug 27, 2024
36496b0
lint changelog
roycald245 Aug 28, 2024
606b118
Merge branch 'main' into feat/add-http-method
Kielek Aug 29, 2024
34e7fa3
Add http method normalization
roycald245 Sep 1, 2024
128ac4a
Merge branch 'main' into feat/add-http-method
roycald245 Sep 1, 2024
ae37251
Merge branch 'main' into feat/add-http-method
roycald245 Sep 3, 2024
4ea758b
Merge branch 'main' into feat/add-http-method
cijothomas Sep 6, 2024
b56b68f
allow RequestDataHelper configuration by environment variable
roycald245 Sep 8, 2024
b1cdc34
Revert allow environment configuration of RequestDataHelper
roycald245 Sep 9, 2024
06d8c26
Merge branch 'main' into feat/add-http-method
roycald245 Sep 9, 2024
7498fde
Revert revert of requestDataHelper parameter
roycald245 Sep 9, 2024
1c59dd0
Merge branch 'main' into feat/add-http-method
Kielek Sep 9, 2024
1241783
Merge branch 'main' into feat/add-http-method
roycald245 Sep 12, 2024
4c0f465
Merge branch 'main' into feat/add-http-method
cijothomas Sep 19, 2024
7cafdcf
Merge branch 'main' into feat/add-http-method
Kielek Sep 24, 2024
0010779
Merge branch 'main' into feat/add-http-method
Kielek Oct 3, 2024
0e07997
Direct feedback
Kielek Oct 3, 2024
81830ed
typo fix
Kielek Oct 3, 2024
7b7c123
Merge branch 'main' into feat/add-http-method
Kielek Oct 3, 2024
511702f
Merge branch 'main' into feat/add-http-method
roycald245 Oct 3, 2024
76fb410
Merge branch 'main' into feat/add-http-method
Kielek Oct 4, 2024
2529b1b
Merge branch 'main' into feat/add-http-method
roycald245 Oct 5, 2024
946aca3
Merge branch 'main' into feat/add-http-method
roycald245 Oct 11, 2024
49b45d8
Merge branch 'main' into feat/add-http-method
roycald245 Oct 13, 2024
5c1b72e
Merge branch 'main' into feat/add-http-method
roycald245 Oct 14, 2024
a3f94ea
Merge branch 'main' into feat/add-http-method
roycald245 Oct 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,22 +64,35 @@ public static bool HasStarted(HttpContext context, out Activity? aspNetActivity)
PropagationContext propagationContext = textMapPropagator.Extract(default, context.Request, HttpRequestHeaderValuesGetter);

KeyValuePair<string, object?>[]? tags;
Kielek marked this conversation as resolved.
Show resolved Hide resolved
if (context.Request?.Unvalidated?.Path is string path)
{
tags = cachedTagsStorage ??= new KeyValuePair<string, object?>[1];
string? path = context.Request?.Unvalidated?.Path;
string? method = context.Request?.HttpMethod;
Copy link
Member

@vishweshbankwar vishweshbankwar Aug 29, 2024

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed 34e7fa3

Copy link
Author

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.


tags[0] = new KeyValuePair<string, object?>("url.path", path);
}
else
// Determine the number of available tags and initialize the array accordingly
int tagCount = (path is not null ? 1 : 0) + (method is not null ? 1 : 0);
tags = tagCount > 0 ? cachedTagsStorage ??= new KeyValuePair<string, object?>[tagCount] : null;

int tagIndex = 0;
if (tags is not null)
{
tags = null;
if (path is not null)
{
tags[tagIndex++] = new KeyValuePair<string, object?>("url.path", path);
}

if (method is not null)
{
tags[tagIndex] = new KeyValuePair<string, object?>("http.request.method", method);
}
}

Activity? activity = AspNetSource.StartActivity(TelemetryHttpModule.AspNetActivityName, ActivityKind.Server, propagationContext.ActivityContext, tags);

if (tags is not null)
{
tags[0] = default;
for (int i = 0; i < tags.Length; i++)
{
tags[i] = default(KeyValuePair<string, object?>);
}
}

if (activity != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@

## Unreleased

* `TelemetryHttpModule` will now pass the `url.path` tag (set to
* `TelemetryHttpModule` will now pass the `http.request.method` and the `url.path` tags (set to

Check failure on line 5 in src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/CHANGELOG.md

View workflow job for this annotation

GitHub Actions / lint-md / run-markdownlint

Line length

src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/CHANGELOG.md:5:81 MD013/line-length Line length [Expected: 80; Actual: 95] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[Request.Unvalidated.Path](https://learn.microsoft.com/dotnet/api/system.web.unvalidatedrequestvalues.path))
when starting `Activity` instances for incoming requests so that it is
available to samplers and may be used to influence the sampling decision made
by [custom
implementations](https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/trace/extending-the-sdk#sampler).
([#1871](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1871))
([#2023](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2023))
roycald245 marked this conversation as resolved.
Show resolved Hide resolved

## 1.9.0-beta.1

Expand Down
5 changes: 3 additions & 2 deletions src/OpenTelemetry.Instrumentation.AspNet/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,9 @@ below.
> OpenTelemetry has the concept of a
[Sampler](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#sampling).
When using `OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule` the
`url.path` tag is supplied automatically to samplers when telemetry is started
for incoming requests. It is recommended to use a sampler which inspects
`url.path` in addition to `http.request.method` tag are supplied automatically
to samplers when telemetry is started for incoming requests.
It is recommended to use a sampler which inspects
`url.path` (as opposed to the `Filter` option described below) in order to
perform filtering as it will prevent child spans from being created and bypass
data collection for anything NOT recorded by the sampler. The sampler approach
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ public void AspNetRequestsAreCollectedSuccessfully(
new TestSampler(SamplingDecision.RecordAndSample)
{
ExpectedUrlPath = expectedUrlPath,
ExpectedHttpRequestMethod = expectedRequestMethod,
})
.Build())
{
Expand Down Expand Up @@ -314,6 +315,8 @@ private class TestSampler(SamplingDecision samplingDecision) : Sampler

public string? ExpectedUrlPath { get; set; }

public string? ExpectedHttpRequestMethod { get; set; }

public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
{
if (!string.IsNullOrEmpty(this.ExpectedUrlPath))
Expand All @@ -322,6 +325,12 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame
Assert.Contains(samplingParameters.Tags, t => t.Key == "url.path" && (t.Value as string) == this.ExpectedUrlPath);
}

if (!string.IsNullOrEmpty(this.ExpectedHttpRequestMethod))
{
Assert.NotNull(samplingParameters.Tags);
Assert.Contains(samplingParameters.Tags, t => t.Key == "http.request.method" && (t.Value as string) == this.ExpectedHttpRequestMethod);
}

return new SamplingResult(this.samplingDecision);
}
}
Expand Down
Loading