Update HttpInListener to add gRPC tags - By creating a new activity with the OperationName used by the framework

[utpilla]

Fixes #1740

Changes

• Updated OnStartActivity method to create a sibling activity using the same OperationName as that of the activity created by framework. This leads to the framework adding the grpc.method and grpc.status_code tags to the activity created by the instrumentation

• Added a boolean custom property called IsCreatedByInstrumentation to know if the activity was created by instrumentation

• Add a tag IsCreatedByInstrumentation and set it to bool.TrueString right after creating the activity in the listener so that we only have to check the first tag to determine if the activity was created by instrumentation

• Stop the activity created by instrumentation in the OnStopActivity method by checking the newly added tag IsCreatedByInstrumentation

• Updated AddAspNetCoreInstrumentation extension method to not add ActivityCreatedByHttpInListener as a legacy source

• Added a unit test for the gRPC scenario where the instrumentation creates a new activity

• Added an ActivityExtension method named TryCheckFirstTag

• Updated CHANGELOG.md

[codecov]

Codecov Report

Merging #1879 (58a062d) into main (073ce6c) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1879      +/-   ##
==========================================
+ Coverage   83.88%   83.92%   +0.03%     
==========================================
  Files         192      192              
  Lines        6181     6195      +14     
==========================================
+ Hits         5185     5199      +14     
  Misses        996      996              

Impacted Files	Coverage Δ
...tion.AspNetCore/TracerProviderBuilderExtensions.cs	100.00% <ø> (ø)
...Telemetry.Api/Internal/ActivityHelperExtensions.cs	100.00% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs	86.42% <100.00%> (+0.19%)	⬆️

[CodeBlanch]

It is super expensive to do this if we're the first caller to add a custom property. We used to add resource as one, and did a bunch of work to avoid that allocation. Can we add as a TagObject instead and then remove after we check it? Adding tag is also not ideal because you have to loop over tags to find one, but if we add immediately after creation, it will be first!

[utpilla]

This is not a common scenario. If we use a tag to keep track of this, we will have to remove this tag before activity.Stop so that this tag does not get exported. This would mean that we will have to look through the TagObjects for this tag for all scenarios. The most commonly occurring scenario would be where there is no new activity created and this tag does not get added. We will have to iterate through all the TagObjects to confirm that no new activity was created which might be a little more expensive than looking up a custom property.

I feel the customProperty approach is good to unblock and fix this issue. We need to run benchmark tests for customProperties vs TagObjects approach to find the most performant one. I have created an issue #1887 to track this

[CodeBlanch]

If the user is using some form of propagation other than W3C will it fire 100% of the time? Rare I think would be like it only happens for connection failures.

Some other (maybe crazy) ideas I came up with trying to think outside the box:

• The AspNetCore created Activity never has a parent? What if we set a parent on the one we create and use that as the signal?

• AspNetCore created Activity doesn't have an ActivitySource set? What if we set one to use as the signal? It is a private set so we would need a bit of magic for that.

[alanwest]

If the user is using some form of propagation other than W3C will it fire 100% of the time?

Yes, this is true. Also, B3 is pretty common case, so yea if there are perf concerns here then it makes sense to me to consider this.

The AspNetCore created Activity never has a parent?

I do not think this is always true. AspNetCore created activity will have a parent if the calling service included W3C context headers in the request. Though not sure how common it would be to have both W3C and B3 headers.

AspNetCore created Activity doesn't have an ActivitySource set? What if we set one to use as the signal? It is a private set so we would need a bit of magic for that.

This seems like a legit solution. AspNetCore may eventually get instrumented with ActivitySource, but then I guess this instrumentation would not be invoked, so we're probably good.

[pcwiese]

Is there something blocking this PR?

[cijothomas]

Assigned to Grpc expert @alanwest for further review.

[alanwest]

@utpilla The fix seems good. I agree with @CodeBlanch we should consider perf implications. I think the ActivitySource check has some good potential.

[CodeBlanch]

LGTM

[cijothomas]

LGTM.
Blocking to address couple of comments. Please address them too and also add changelog entry.