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]

The committers listed above are authorized under a signed CLA.

• ✅ login: Wraith2 / name: Wraith (accefcb, f668437, 63ec26d)

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

[cijothomas]

instrumentations are generally not doing this due to concern over heap allocation. This must, however, be solved, and need to work with the owners (ActivitySource/Activity i.e runtime team) to see if they can support TagList or StartActivity overloads for taking 1,2,3 tags, so this allocation can be avoided.

[Wraith2]

I couldn't find a good way to deal with the allocation. The api requires IEnumerable so whatever happens it either has be a class or it'll be boxed. I tried TagList but decided the boxing of such a large struct was going to be more expensive than what the compiler is likely to do here, which is probably a new single length array. I considered creating a class which can contain the single element and which is enumerable but it felt like overkill for a first contribution to the project.

[cijothomas]

it is not possible to avoid this, without changes in the API offered by runtime itself.

The only instrumentation in the repo providing tags at startup is the sqlclient one, but only does so for fully static tag: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs#L25

[cijothomas]

instrumentations are generally not doing this due to concern over heap allocation. This must, however, be solved,

@Wraith2 Just to make it clear - I didn't mean that this should be solved in this PR or you should work with runtime team to solve this. Just stated that this is a problem that must be solved. (I can create a tracking issue in runtime and otel repos, once i find the old discussion/probably issue itself asking the same)

OTel convention requires a lot of tags to be provided at startup time itself, and it is not possible to do it without allocation in Activity, so this is a problem that needs to be solved even outside of this issue/pr.

[Wraith2]

Understood. The allocation was something that I spent more time on than the fairly easy code change and I just couldn't find a more efficient way to do it. I've improved performance a lot in SqlClient by being careful around allocations so having to add an allocation here for such a trivial case bothered me.

If it does turn out that work is needed in dotnet/runtime I may be able to help there because I'm already a contributor to that repo.

[cijothomas]

Couldn't find a tracking issue, but dotnet/runtime#42784 (comment) and dotnet/runtime#42784 (comment) has explained the problem. If you are up for it, please create a new issue in the runtime repo. Else, I can create one.

(Also, it is very unlikely that runtime will be able to make any changes for .NET 9 timeframe, so we are looking at .NET 10 timeframe only at this point)

[Wraith2]

New proposal for a CreateActivity overload that takes a single kvp, dotnet/runtime#103245

[CodeBlanch]

Could we use a ThreadStatic for the storage?

    [ThreadStatic]
    private static KeyValuePair<string, object?>[]? samplerTagStorage;

        IEnumerable<KeyValuePair<string, object?>>? tags;
        if (context.Request?.Unvalidated?.Path is string path)
        {
            var samplerTags = samplerTagStorage ??= new KeyValuePair<string, object?>[1];
            samplerTags[0] = new KeyValuePair<string, object?>("url.path", path);
            tags = samplerTags;
        }
        else
        {
            tags = null;
        }

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

Pretty sure sampling happens on the same thread so it is mostly safe. Some user could make a custom sampler that attempts to hold onto the tags but why would they?

[Wraith2]

The tags will get added to the TagList in the activity and since KeyValuePair is a struct they're copied and the reference to the incoming array is not retained. So it'll be safe as long as no await is added along the current path.

I've added this change.

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

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...

opentelemetry-dotnet-contrib/build/Common.props

Line 46 in e3a90d7

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

...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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.36%. Comparing base (71655ce) to head (f668437).
Report is 362 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1871      +/-   ##
==========================================
+ Coverage   73.91%   75.36%   +1.45%     
==========================================
  Files         267       15     -252     
  Lines        9615      341    -9274     
==========================================
- Hits         7107      257    -6850     
+ Misses       2508       84    -2424     

Flag	Coverage Δ
unittests-Instrumentation.AspNet	75.36% <100.00%> (?)

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

Files	Coverage Δ
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs	87.50% <100.00%> (+2.25%)	⬆️
...umentation.AspNet/Implementation/HttpInListener.cs	88.15% <ø> (-0.16%)	⬇️

... and 269 files with indirect coverage changes

[github-actions]

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

[cijothomas]

is there a need to do this reset?

[cijothomas]

LGTM.
Please add changelog entry, given this is end user facing improvement.
Also, it'd be good to see if this can be covered via unit tests, esp. the TLocal part.

[github-actions]

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