Opentelemetry-dotnet: Don't record child activities

[ejsmith]

I am trying to create an instrumentation for the Elasticsearch.Net client. Internally, it's using HttpClient. I'd like to only record the Elasticsearch call and the details about it and have it show up as a db.system of type elasticsearch. I've got that much working, but the problem is that it's also recording the internal HttpClient call. Is there some way I can cancel recording all child activities after starting the Elasticsearch call activity so that it won't capture the HttpClient call? I know that I can provide a filter function for the HttpClient instrumentation, but I was really hoping to not have to do that.

[cijothomas]

@alanwest faced the same issue Grpc, which is on top of httpclient. We need to solve the problem in generic way to address both these and other future scenarios.
(I don't know of a perfect solution, but will come back to this next week)

[reyang]

Relates to #809

[reyang]

@cijothomas is this duplicated with #809 or a different thing?

[felpasl]

same here, try to send trace to Linkerd (OpenCensus Collector) using Zipkin exporter and stuck on 100% CPU because of a loop in trace zipkin call

[cijothomas]

@cijothomas is this duplicated with #809 or a different thing?

Related, yes. But not exact duplicate. 809 is for users to filter early. This issue if more for InstrumentationLibrary authors.

[ejsmith]

@cijothomas exactly. I've been looking through the code and I'm not really sure how we could go about this other than some sort of ability for a parent activity to be able to preview and filter out any child activities. Not even sure how that would work since Activity is owned by the .NET team.

[alanwest]

There is some discussion at the spec level here https://github.com/open-telemetry/opentelemetry-specification/issues/530. Seems like a natural solution is to do something like what @reyang did in Python by introducing a suppress_instrumentation flag on the context: https://github.com/open-telemetry/opentelemetry-python/pull/181.

[CodeBlanch]

I've been harping on this since November! Glad to see everyone finally coming on board 🤣

[reyang]

I've been harping on this since November! Glad to see everyone finally coming on board 🤣

I've added this to OpenCensus 3 years ago and still having trouble today convincing people it is important 😄

[ejsmith]

Yikes. That doesn't sound promising.

[alanwest]

I've added this to OpenCensus 3 years ago and still having trouble today convincing people it is important 😄

Ugh, really? Having worked on observability products for over four years now, this just seems like basic functionality to me. @reyang @CodeBlanch Is there a particular github issue or venue (other than the ones already noted here) at the spec-level where we could best be chiming in on this discussion? I'd be more than happy to add my voice.

[reyang]

Ugh, really? Having worked on observability products for over four years now, this just seems like basic functionality to me. @reyang @CodeBlanch Is there a particular github issue or venue (other than the ones already noted here) at the spec-level where we could best be chiming in on this discussion? I'd be more than happy to add my voice.

https://github.com/open-telemetry/opentelemetry-specification/issues/530

The issue has been tagged as release:after-ga, if we really want to make progress in the spec, we should first make it as a priority and part of the GA requirement. Personally I think this is unlikely going to happen, as from the spec perspective I don't see a good reason to take it, instead of leaving it to individual language SDKs.

[reyang]

Given I have done this several times in different languages, I won't be afraid to do it again in this repo 😄
If I got enough 👍 I am happy to send a small PR on this:

1. I think we can do this in the OpenTelemetry API/SDK.
2. I think it is unlikely to get this in the spec before GA, but we can try.
3. I don't think we should/can push .NET to make this as part of the .NET framework API.

[alanwest]

If I got enough 👍 I am happy to send a small PR on this

I've attempted to give you all the affirmative emojis github will allow me.

[CodeBlanch]

@reyang sounds good to me. Are you going to do an AsyncLocal context thing? FYI @SergeyKanzhelev sent me this https://github.com/lmolkova/oteps/blob/terminal-context/text/0069-terminal-context.md (by @lmolkova) a while back when we were discussing possible solutions.

[reyang]

@reyang sounds good to me. Are you going to do an AsyncLocal context thing?

I think it needs to be an abstract interface where people can swap the actual implementation. Something similar to this https://github.com/census-instrumentation/opencensus-python/tree/master/context/opencensus-context#opencensus-runtime-context.
And yes, on modern version of .NET/Core runtime, it should be AsyncLocal by default.

[cijothomas]

@rajkumar-rangaraj is working to make this configurage in gRPC client instrumentation. Once done we can review it and make it the recommendation for every instrumentation authors.

[reyang]

I think this should be addressed by #1202 and #1184.