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.

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

Ability to disable creation of intermediate activities in DiagnosticsHandler #30392

Closed
a-elsheikh opened this issue Feb 23, 2021 · 7 comments · Fixed by #33777
Closed

Ability to disable creation of intermediate activities in DiagnosticsHandler #30392

a-elsheikh opened this issue Feb 23, 2021 · 7 comments · Fixed by #33777
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Needs: Design This issue requires design work before implementating.
Milestone

Comments

@a-elsheikh
Copy link

a-elsheikh commented Feb 23, 2021

The Problem

AspNetCore services are creating intermediate activities during HTTP comms, when invoking the service from any external component that has telemetry instrumentation the traces end up as broken hierarchies as unintended child spans are being created in the aspnetcore service, especially if said service sits as a bridge between 2 components.

for example, given that services A, C and D are instrumented but not B

A -> B - parent abc span def
B -> C - parent def span ghi
C -> D - parent ghi span jkl

if I have no intention of instrumenting B then preferably it won't create a span, otherwise the exported trace ends up broken and instead of looking like this

abc / def / ghi / jkl /

it will look like this

abc / def /
jkl /

additionally, this creates what can be deemed as unnecessary noise in the tracing and is potentially a performance consideration for highly optimised high throughput systems (questionable).

Suggested Solution

I'm not sure of the implications this would have on users relying on this as a default behaviour so perhaps an opt-in may not be viable but at least the ability to opt out.

Additional Context

Personally I experienced this issue when using Dapr sidecars. For the most part, the sidecars themselves emit the desired telemetry however the traces all end up broken when invoking aspnetcore services because of the elusive spans.

Intermediate activities/spans not exported
image

Intermediate activities/spans exported
image

@a-elsheikh a-elsheikh changed the title Ability to disable creation of intermediate activities in DiagnosticHandler Ability to disable creation of intermediate activities in DiagnosticsHandler Feb 23, 2021
@davidfowl
Copy link
Member

cc @shirhatti @noahfalk

@rynowak
Copy link
Member

rynowak commented Feb 23, 2021

@davidfowl - I mentioned this to you offline. I expect that this means ASP.NET Core apps always need to do their own tracing - even in setups where you are trying to avoid that as a requirement (service mesh, Dapr).

@a-elsheikh - thanks for opening this - do you mind including the screenshots you shared in chat? I can dig them up if you can't find them. I think it makes what's going on really clear.

@noahfalk
Copy link
Member

The scenario request makes sense to me, the tricky part is figuring out how we want to model it and configure it. The best option that currently comes to mind would be to reuse Activity and Activity.Current, but make the local Activity id exactly match the parent id. Currently there is no BCL API to create an Activity that proxies a remote parent, we always make a new child that is a descendant of the remote parent. We'd need configuration options on both ingress and egress that changes the default policy of that code, which currently is to create a new child Activity.

The other issue we'd need to look out for in this scenario is that in order to be a transparent proxy, the user would need to avoid having any listener on that node which triggers creation of new Activities. For example currently Kestrel assumes that if logging is on then activity creation also needs to be on and we could control for that one. However you could imagine some other logging mechanism registering itself as an ActivityListener and that would be out of .NET's control. As soon as any component is requesting Activities get created but not recording them in the distributed logging system then you get an incomplete view.

Another alternative/short term mitigation is to use the TraceState header. The W3C spec has some examples of what they think logging would like where different parts of a distributed system are monitored by different backends, thus neither one of them has a complete view. In short TraceState can pass along a hint about what was the last spanId in the chain that was recorded so that it can still be linked up across the gap.

@a-elsheikh
Copy link
Author

issue updated with screenshots

@noahfalk
Copy link
Member

@tarekgh

@BrennanConroy BrennanConroy added this to the Next sprint planning milestone Mar 3, 2021
@ghost
Copy link

ghost commented Mar 3, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@BrennanConroy BrennanConroy added the Needs: Design This issue requires design work before implementating. label Mar 3, 2021
@BrennanConroy
Copy link
Member

Triage: @tarekgh can you work with @shirhatti on a design for this.

@shirhatti shirhatti linked a pull request Jul 1, 2021 that will close this issue
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Needs: Design This issue requires design work before implementating.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants