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.

Sign up for GitHub

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

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

Merged
merged 27 commits into from
May 4, 2021
Merged

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

Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
db21758
Update HttpInListener to add gRPC tags
utpilla Mar 8, 2021
6a679cf
Update ASP.NET Core instrumnetation to use the OperationName used by …
utpilla Mar 8, 2021
84e61b1
Merge remote-tracking branch 'origin/main' into utpilla/Fix-gRPC-Inst…
utpilla Mar 8, 2021
2b0271f
Address PR comments
utpilla Mar 8, 2021
de2d299
Merged with origin/main
utpilla Mar 9, 2021
8f026fc
Correct merge with origin/main
utpilla Mar 10, 2021
bcc6ea8
Merge remote-tracking branch 'fork/utpilla/Fix-gRPC-Instrumentation-F…
utpilla Mar 10, 2021
8b32725
Fix merge error
utpilla Mar 10, 2021
aef69cd
Merge remote-tracking branch 'origin/main' into utpilla/Fix-gRPC-Inst…
utpilla Mar 10, 2021
b5765c4
Merge branch 'main' into utpilla/Fix-gRPC-Instrumentation-For-Sibling…
utpilla Mar 18, 2021
aabfc8d
Resolve merge conflict
utpilla Mar 19, 2021
a53199f
Merge remote-tracking branch 'fork/utpilla/Fix-gRPC-Instrumentation-F…
utpilla Mar 19, 2021
91e52c6
Fix formatting
utpilla Mar 19, 2021
642842e
Merge branch 'main' into utpilla/Fix-gRPC-Instrumentation-For-Sibling…
utpilla Apr 12, 2021
a1b58d1
Merge with origin/main
utpilla Apr 26, 2021
214c4f9
Merge branch 'utpilla/Fix-gRPC-Instrumentation-For-Sibling-Activity-U…
utpilla Apr 26, 2021
b872349
Use tag instead of custom property in the HttpListener; Added CheckFi…
utpilla Apr 27, 2021
6e5da33
Set tag value as True instead of boolean true
utpilla Apr 27, 2021
3a068ea
Use bool.TrueString instead of True
utpilla Apr 27, 2021
a03f7d1
Merge branch 'main' into utpilla/Fix-gRPC-Instrumentation-For-Sibling…
cijothomas May 3, 2021
963f930
Merge remote-tracking branch 'origin/main' into utpilla/Fix-gRPC-Inst…
utpilla May 3, 2021
12222e4
Address PR comments
utpilla May 3, 2021
39f60a4
Trigger CI Run
utpilla May 3, 2021
44383ad
Merge remote-tracking branch 'origin/main' into utpilla/Fix-gRPC-Inst…
utpilla May 3, 2021
33fd687
Merge branch 'main' into utpilla/Fix-gRPC-Instrumentation-For-Sibling…
cijothomas May 3, 2021
6405fbd
Update CHANGELOG.md
utpilla May 3, 2021
58a062d
Merge branch 'utpilla/Fix-gRPC-Instrumentation-For-Sibling-Activity-U…
utpilla May 3, 2021
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
9 changes: 8 additions & 1 deletion src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,15 @@ public override void OnStartActivity(Activity activity, object payload)
// Create a new activity with its parent set from the extracted context.
// This makes the new activity as a "sibling" of the activity created by
// Asp.Net Core.
Activity newOne = new Activity(ActivityNameByHttpInListener);
Activity newOne = new Activity(ActivityOperationName);
newOne.SetParentId(ctx.ActivityContext.TraceId, ctx.ActivityContext.SpanId, ctx.ActivityContext.TraceFlags);
newOne.TraceStateString = ctx.ActivityContext.TraceState;

// Starting the new activity make it the Activity.Current one.
newOne.Start();

newOne.SetCustomProperty("IsCreatedByInstrumentation", true);
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@alanwest alanwest Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.


// Set IsAllDataRequested to false for the activity created by the framework to only export the sibling activity and not the framework activity
activity.IsAllDataRequested = false;
activity = newOne;
Expand Down Expand Up @@ -211,6 +213,11 @@ public override void OnStopActivity(Activity activity, object payload)
// the one created by the instrumentation.
// And retrieve it here, and set it to Current.
}

utpilla marked this conversation as resolved.
Show resolved Hide resolved
if (activity.GetCustomProperty("IsCreatedByInstrumentation") is bool isCreatedByInstrumentation && isCreatedByInstrumentation)
{
activity.Stop();
}
}

public override void OnCustom(string name, Activity activity, object payload)
Expand Down