TelemetryHttpModule add url.path to tags

[Wraith2]

Added the "url.path" tag to the tags for newly created activities in StartAspNetActivity.

To ignore a path like a healthcheck it is best to use a sampler to ensure that the activity and all child activities are dropped in a sampler. To do this the sampler needs to be able to provide enough information for the sampler author to detect the path. This is only currently possible by using HttpContext.Current or other method of injecting the request into the sampler. This is unsatisfying because the caller of the sampler has information about the current request it just doesn't have a way to pass that information to the sampler.

This PR changes the way StartAspNetActivity works by adding the request.Unvalidated.Path of the request to the tags that are used to construct the activity. The tags are one of the few things that are passed to samplers when an activity is started. With this change it is now possible to write a sampler which uses only provided context information in the sampler to decide whether a particular request root activity should be dropped.

There are no public api changes.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: Wraith2 / name: Wraith (adeb5cfca1ad4bc79a960b2db2e837cfcea3c6ba, bec8e521f618205a748335d3b9bcb1b525e8a478, 7b6fc26a600b6dfce24378cfe3d016138c0c1cd0, 58208b9ace617839dfd1cbb50cb3db4936c55a94)
• :white_check_mark: login: cijothomas / name: Cijo Thomas (2ff8657c251954550aaedbbd452b5e5d1185a257, 38c5496e8caa91b83e42dc8f43f3a6d447ad9f22, 87cbdafbfecbccd081cd74950858a4b8b46c137d, 0169f6a99b6530082f112c222c90317eeb012284)
• :white_check_mark: login: CodeBlanch / name: Mikel Blanchard (d84ead23ce3600606ad86cff802021a4c7e07964, 417835b59cda4e5a57ed34b4c05be7536ef117dd, c96a57b1d498a1854d1066f55a2906188768d00d, fa6d187c6d1a9a09cbb2382736b6498fa73fa343)

[martinjt]

I think as we're adding this, it would be better to add as much as we can, not just the path, anything we have that's available at that time.

What about the code that adds these properties later, should they be removed?

I'm also curious about whether constants for the keys would be better.

[Wraith2]

I'm also curious about whether constants for the keys would be better.

Using one of the existing constants would require that I add a project reference to the OpenTelemetry.SemanticConventions assembly. Adding a local constant seemed pointless when this is the only use of it in this library, if it were multiuse in this project I'd have made it a constant. If you prefer a constant or want me to add the assembly dependency let me know which you'd like.

I think as we're adding this, it would be better to add as much as we can, not just the path, anything we have that's available at that time.

Very little is available at this time because it's very early in the request lifecycle. No validation or parsing has been done yet which is why I'm using the Unvalidated property of the request. All the attributes which are added later are added by the AspNet integration not by the telemetry module and they require various amounts of validation and parsing. The only other possibly useful information I can think of would the the route that is used but it isn't and can't be made available at this time in the lifecycle. Things like http version, http method really don't seem like useful attributes for making a sampling decision.

[Wraith2]

Just for clarity, what is this waiting on? and if it's me can you tell me what is needed?

[Wraith2]

D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\src\OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule\ActivityHelper.cs(68,26): warning SA1010: Opening square brackets should not be preceded by a space [D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\src\OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule\OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.csproj]

This is unavoidable due to conflicting rules. One rule requires a space and another requires there to be no space so both cannot be satified. enumerable = [new KeyValuePair<string, object?>("url.path", path)]; gives SA1010 An opening square bracket within a C# statement is not spaced correctly. applying the fixer gives: enumerable =[new KeyValuePair<string, object?>("url.path", path)]; gives SA1003 The spacing around an operator symbol is incorrect, within a C# code file

[cijothomas]

Just for clarity, what is this waiting on? and if it's me can you tell me what is needed?

I'll let others review, but my concern is the allocation in hot path. It might be okay because of the fact that Activity itself will be created (heap allocated) irrespective of sampling decision (as it is root), so adding few more bytes (for the tag) may not be critical. Also once runtime supports passing tags without allocation (in .NET 10 timeframe), this won't be an issue.

[Wraith2]

'm also curious about whether constants for the keys would be better.

Using one of the existing constants would require that I add a project reference to the OpenTelemetry.SemanticConventions

Looking at AspNet is includes SemanticConventions by linking to the file. Is there a specific reason for this? I expected there to be a nuget package containing public versions of the convention constants but there doesn't seem to be one. I can add a link to the shared file and use the convention constant definitions but it seems like bloat to do so.

[martinjt]

We're working on a method for producing the conventions long-term and what that will look like, but's not ready yet. It's an ongoing task for all the languages.

[martinjt]

In terms of the wider idea of the allocations, my suggestion to sidestep this if it's a concern is to make it opt-in by adding a parameter that will do it instead of doing it by default.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[Wraith2]

That's a pretty aggressive stale marking behaviour. Is there a specific thing blocking this PR?

[CodeBlanch]

This is unavoidable due to conflicting rules. One rule requires a space and another requires there to be no space so both cannot be satified. enumerable = [new KeyValuePair<string, object?>("url.path", path)]; gives SA1010 An opening square bracket within a C# statement is not spaced correctly. applying the fixer gives: enumerable =[new KeyValuePair<string, object?>("url.path", path)]; gives SA1003 The spacing around an operator symbol is incorrect, within a C# code file

@Wraith2

Bump this...

https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/e3a90d7ac5b0cb50ebe9838c87a6e6fe06d4dd40/build/Common.props#L46

...to...

-    <StyleCopAnalyzersPkgVer>[1.2.0-beta.507,2.0)</StyleCopAnalyzersPkgVer>
+    <StyleCopAnalyzersPkgVer>[1.2.0-beta.556,2.0)</StyleCopAnalyzersPkgVer>

The new StyleCop version seems to do the right thing and stops warning.

[CodeBlanch]

@martinjt

In terms of the wider idea of the allocations, my suggestion to sidestep this if it's a concern is to make it opt-in by adding a parameter that will do it instead of doing it by default.

Seems like making it an opt-in thing via options is reasonable. What would that option look like though? The spec defines a bunch of tags for the sampler:

client.address http.request.header. http.request.method server.address server.port url.path url.query url.scheme user_agent.original

We probably don't want an option for everything. Could be a flags enum or HashSet or something?

[Wraith2]

The new StyleCop version seems to do the right thing and stops warning.

I've rebased which should pick up the new stylecop.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.00%. Comparing base (71655ce) to head (0169f6a). Report is 383 commits behind head on main.

Files	Patch %	Lines
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs	85.71%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1871      +/-   ##
==========================================
+ Coverage   73.91%   77.00%   +3.09%     
==========================================
  Files         267       15     -252     
  Lines        9615      361    -9254     
==========================================
- Hits         7107      278    -6829     
+ Misses       2508       83    -2425     

Flag	Coverage Δ
unittests-Instrumentation.AspNet	77.00% <85.71%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...umentation.AspNet/Implementation/HttpInListener.cs	88.15% <ø> (-0.16%)	:arrow_down:
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs	85.50% <85.71%> (+0.26%)	:arrow_up:

... and 269 files with indirect coverage changes

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[Wraith2]

I've added a changelog entry.

I looked at how to add tests and couldn't find a good way to do it. The Opentelemetry.Trace namespace isn't available in the HttpInListener test project and it isn't clear how to add the reference to the correct version from the other projects around it. The presence of url.path is already tested in the existing asp.net tests.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[Wraith2]

Disappointing.

[cijothomas]

Disappointing.

Don't be! The automation just closes PRs if not active for 7 days! I'll reopen now and request reviews again. (Some of the maintainers were on break for last few days, but mostly back now I think.)

[roycald245]

I'm also curious about whether constants for the keys would be better.

Using one of the existing constants would require that I add a project reference to the OpenTelemetry.SemanticConventions assembly. Adding a local constant seemed pointless when this is the only use of it in this library, if it were multiuse in this project I'd have made it a constant. If you prefer a constant or want me to add the assembly dependency let me know which you'd like.

I think as we're adding this, it would be better to add as much as we can, not just the path, anything we have that's available at that time.

Very little is available at this time because it's very early in the request lifecycle. No validation or parsing has been done yet which is why I'm using the Unvalidated property of the request. All the attributes which are added later are added by the AspNet integration not by the telemetry module and they require various amounts of validation and parsing. The only other possibly useful information I can think of would the the route that is used but it isn't and can't be made available at this time in the lifecycle. Things like http version, http method really don't seem like useful attributes for making a sampling decision.

Actually it would be nice to have the http method :) I am in need of the ability to have the http properties in the sampler. but the difference is that i require the http method in addition to the url. Would be nice if this can be added to this PR and not to make another just for another property :)

[cijothomas]

@roycald245 It is reasonable to add more tags, esp. since semantic convention recommends that. It requires some opt-in design model, that is not yet in. We can track it separately (please open an issue), to not delay this PR.

[roycald245]

@cijothomas opened #57287. Will open PR as soon as this merges