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 (0d797a60b44c081e2e6aa7000f7a597ba2bef3b2, 39ccd687af9300e5a7129ed344a2bdb3c6dfbb64)

[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?