-
Notifications
You must be signed in to change notification settings - Fork 760
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
Filter activites proposals. #809
Comments
I think we need to clarify the scenario first, here are the scenarios that I can see:
For 1), OpenTelemetry Python have specific rules for different instrumentation/extension, for example https://github.com/open-telemetry/opentelemetry-python/blob/2a952b3d4540fa767974c48afdb781d9e9340edd/ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py#L68 For 2) OpenTelemetry Python has a context flag suppress_instrumentation. |
Sorry accidentally closed the issue. Well-put @reyang! Agree those are the 2 scenarios at play here. |
This is not really about a client side request, right? This seems more about filtering requests server side. If we consider the proposed API then, for ASP.NET core for example, I think we'd want something like:
Perhaps we can either think of a way to generalize the idea of the FilterFunc to accommodate arbitrary payloads based on instrumentation. Or maybe it just makes sense to focus on the more narrow use case and just have a list of URLs as it appears python has done with |
It is just one example, the scenario could cover both client/outgoing request and server/incoming request.
I'm fine with either way as long as perf is reasonable (e.g. we don't do reflections and sacrifice performance in order to be generic). |
I found a few examples (I think) in other OTel repos: I would like to move ahead with the below additions (on #741) for Http outgoing calls, we can tackle incoming calls after that? The perf will be more or less what it is today for the filter, because they already run through a filter, it's just not public. For the "StartAction" the perf hit when not used will be a null check. When in use, perf will be up to the user as they are driving it. Public API additions to /// <summary>
/// Gets or sets an optional callback method for filtering HttpClient requests that are sent through the instrumentation.
/// </summary>
public Func<HttpRequestMessage, bool> FilterFunc { get; set; }
/// <summary>
/// Gets or sets an optional callback method that will be invoked after HttpClient Activity objects are started.
/// Useful for adding non-standard tags for collection or changing the <see cref="Activity.DisplayName"/> to include the request url.
/// </summary>
public Action<HttpRequestMessage, Activity> StartAction { get; set; } Same will be added for |
I support you @CodeBlanch. |
|
How would I as the author of an instrumentation that uses HttpClient internally be able to filter out the HttpClient activities without having to tell my users to manually add a request filter to their HttpClient instrumentation options? |
This proposal won't help your scenario. Lets discuss this in the other issue, and find a solution. |
I think both filtering (at instrumentation level) and suppression (based on the runtime context, could happen anywhere including exporter, instrumentation, processor) are solved. |
(copy-pasted from meeting agenda, not formatted correctly. Will edit later.)
Filtering of dependencies from httpclient. 4 approaches to compare and discuss.
1 - Use Activity processor - OT spec approach, not so performant as allocations are still done.
2 - Use Sampler (special one wrapping existing samplers) - OT spec supports this, and this is better than above, as some allocations are saved.
3. Custom solution for HttpClientInstrumentation - can use the filter to DiagnosticSource.IsEnabled itself, ensuring the unwanted activity is blocked from ever being created. This will be a custom .NET solution, not derived from OT spec.
This already exists, but not exposed as public. #741 is proposing to make this public.
[From Mike B on options API: If we decide to modify the options, I would like to also expose a callback action for changing the DisplayName of the Http Activity. We used to have some of the Url on there, and then we changed it to just HttpMethod. The spec says we should use method but may allow for a hook to change that (ref). Sergey brought up on PR 741 possibly adding a callback to allow users to customize the tags. I combined those two things into one callback. Proposal:
]
4. Terminal Activity concept - this should solve the issue for a broader scope, not just http. For eg: If an exporter is simply writing to SQLServer, we don’t want to capture that as telemetry. Solution tied to httpclient won’t fix this.
[Note from Mike B on the above:
#4 is a slightly different solve than what is on the PR. Terminal activity (SpanKind.Terminal, ActivityKind.Terminal, etc) would be set by an exporter to flag its activity as internal to the execution of the OT mission, meaning, not-for-collection. For example, Zipkin exporter should be able to flag its Http transmission of span data as ignored from collection. Today these calls are filtered out in the logic found in HttpClientInstrumentationOptions (same logic being modified on the PR). This will be an on-going issue until we have a proper solve. For example, if you are using Stackdriver exporter, it transmits over gRPC using Google’s library. No issues today. But if someone comes along and instruments that gRPC library, then the transmit of spans creates a new span, which is transmitted, putting us in an infinite (potentially expensive) loop. You could of course build an ActivityProcessor to filter them, but the idea is to have some built-in way to handle this case automatically. Ideas have been to use SpanKind/ActivityKind or some kind of ambient context (like an AsyncLocal) running over the exporter activity that can be checked for exclusion by the instrumentation.
What the PR is seeking to do is give users the ability to filter out anything they don’t want captured, not just internal stuff. It’s a value-add, more or less. In my case, it is some appliance IT runs on our servers which emits Http calls to its agent out of our process. ActivityProcessor & ActivitySampler would work as solutions to this, but with the perf hit of creating an Activity you know you are going to drop/filter. Since there is already a filter executing in HttpClientInstrumentationOptions, it seemed like a good spot (easy to implement, non-perf impacting, perf-improving for filter case) to allow users to participate in the decision. ActivityProcessor or ActivitySampler would also work, but more effort to build than simply setting some options and you are hit with the perf of creating the Activity you don’t want.]
The text was updated successfully, but these errors were encountered: