-
Notifications
You must be signed in to change notification settings - Fork 566
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
Add telemetry filter helper feature so developer code can influence automatic span creation #9552
Add telemetry filter helper feature so developer code can influence automatic span creation #9552
Conversation
…hether incoming or outgoing spans are automatically created
@@ -128,6 +152,10 @@ public void filter(ClientRequestContext clientRequestContext, ClientResponseCont | |||
clientRequestContext.removeProperty(SPAN_SCOPE); | |||
} | |||
|
|||
private static List<HelidonTelemetryClientFilterHelper> helpers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick comment without a lot of research behind it: I wonder if you shouldn't use the native Jersey features for this discovery operation. In CDI this would involve injecting an Instance<...Helper>
at construction time. In "native" Jersey this would probably involve injecting a Providers
to get hold of a ContextResolver<...Helper>
. Rationale: Jersey already has a robust service discovery mechanism defined by spec. On the other hand maybe Helidon service loading has been deemed the architectural path forward here; I don't know.
Another related question I have here is that a helper that votes to not start a span is, notionally, just a filter that does nothing, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good questions.
-
I did not think about that approach.
It would certainly be an alternative and the filters that are already in place are certainly Jakarta REST elements so that would be thematically consistent.
I guess my own thinking of late in general is to use the Helidon mechanism for ease of moving to a hypothetical future alternative. On the surface (I have not tried redoing the PR as you described) it seems as if there are fewer moving parts as written.
-
I suppose, because the existing filter passes the
ClientRequestContext
orContainerReqeustContext
to each helper I suppose a helper could act as a filter by manipulating the context in the same way "real" filters can. That's certainly not the intent.I also considered having the filter pass our own implementation of those Jakarta REST
XXXContext
interfaces that reject the mutator methods and delegate to the real context otherwise. But that seemed excessive (to me) to guard developers from themselves.I did think briefly about--and decided against--having the filter pass a number of parameters to the helper - such as all the items available from the context using the various
getXXX
methods. But that would have been quite cumbersome and there would be no built-in way of allowing access to the properties in the context.I also thought about having developers provide their own Jakarta REST filter and communicate their desire to suppress the span creation by setting a property in the request context. That seems less transparent and would involve priorities for the developers' filters vs. ours so ours runs last; I didn't pursue this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right; the general pattern here is, loosely, you want some Filter-Like-Entities to be able to veto other FLEs (you want those helpers to say to downstream filters of a particular kind "please don't act"). I'm just wondering if there's a more idiomatic way to do it rather than introducing new members into the overall ontology. I'm probably just overthinking it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am leaning toward using the Instance
approach and CDI.
From what I've found, the Jakarta @Provider
approach would only support a single instance of the helper type being made available to the filters; users might have multiple helpers all of which need to participate in each vote.
It would be simpler for developers to annotate their helper implementation classes with a bean-defining annotation (such as @ApplicationScoped
) (as in the CDI approach) than to create the Java service loader file under META-INF/services
(as in the Java service loading approach). And as you pointed out earlier, using Instance
allows the filter to retrieve all the implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only gotcha here is: for every contextual reference you get via Instance#get()
(or next()
) you should arrange to destroy it if and only if it is in @Dependent
scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(from an off-line conversation about the preceding comment)
Conclusion: The filters which find the helpers are not created per-request, so caching the helpers via the Instance
in the filter constructor seems OK here.
Reasoning: The filters last for the life of the application and so therefore any dependent-scoped helpers a developer might write--and which the filters might bring into existence via the Instance
--should have the same lifetime as the filter. Because that's the lifetime of the app, the filter does not really have to explicitly destroy any dependent-scoped helpers and there should not be any memory leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right; the only real-world side effect of not arranging for dependent instances to be destroyed (one way or the other) is that any @PreDestroy
methods they declare will not be executed. This is rarely a problem and I doubt it will crop up in this use case.
…utomatic span creation (helidon-io#9552) * Add telemetry filter helper feature so developer code can influence whether incoming or outgoing spans are automatically created * Add doc describing filter helpers; also fix some Javadoc warnings in related files * Use CDI instead of Java service loading to locate helpers
…utomatic span creation (helidon-io#9552) * Add telemetry filter helper feature so developer code can influence whether incoming or outgoing spans are automatically created * Add doc describing filter helpers; also fix some Javadoc warnings in related files * Use CDI instead of Java service loading to locate helpers
Description
Resolves #9550
Helidon app developers using MicroProfile Telemetry can now add their own implementations of new helper interfaces related to the Jakarta REST client and container filters that create automatic spans for outgoing and incoming traffic. The helpers tell whether or not they want the automatic span created for the given request.
Background
Helidon previously provided container and client filters which always add a new span for the incoming or outgoing call (respectively).
As described in the related issue, sometimes developers want to be able to suppress the filters' automatic creation of the spans.
Changes
This PR adds two new helper SPI interfaces, one for container filters and one for client filters. The new interfaces expose a single method which accepts the corresponding request context (container or client) and returns a
boolean
whether that helper votes to create the automatic span or not.The container and client filters check with all known helpers to find out whether to create the automatic span or not. As written, if at least one helper votes "no" then the span is not created.
The PR also adds an integration test.
Documentation
Doc update included.