-
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
Merged
tjquinno
merged 3 commits into
helidon-io:main
from
tjquinno:4.x-telemetry-filter-selectivity
Dec 10, 2024
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 33 additions & 0 deletions
33
...c/main/java/io/helidon/microprofile/telemetry/spi/HelidonTelemetryClientFilterHelper.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* Copyright (c) 2024 Oracle and/or its affiliates. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package io.helidon.microprofile.telemetry.spi; | ||
|
||
import jakarta.ws.rs.client.ClientRequestContext; | ||
|
||
/** | ||
* Service-loaded type applied while the Helidon-provided client filter executes. | ||
*/ | ||
public interface HelidonTelemetryClientFilterHelper { | ||
|
||
/** | ||
* Invoked to see if this helper votes to create and start a new span for the outgoing client request reflected | ||
* in the provided client request context. | ||
* | ||
* @param clientRequestContext the {@link jakarta.ws.rs.client.ClientRequestContext} passed to the filter | ||
* @return true to vote to start a span; false to vote not to start a span | ||
*/ | ||
boolean shouldStartSpan(ClientRequestContext clientRequestContext); | ||
} |
33 changes: 33 additions & 0 deletions
33
...ain/java/io/helidon/microprofile/telemetry/spi/HelidonTelemetryContainerFilterHelper.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* Copyright (c) 2024 Oracle and/or its affiliates. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package io.helidon.microprofile.telemetry.spi; | ||
|
||
import jakarta.ws.rs.container.ContainerRequestContext; | ||
|
||
/** | ||
* Service-loaded type applied while the Helidon-provided container filter executes. | ||
*/ | ||
public interface HelidonTelemetryContainerFilterHelper { | ||
|
||
/** | ||
* Invoked to see if this helper votes to create and start a new span for the incoming | ||
* request reflected in the provided container request context. | ||
* | ||
* @param containerRequestContext the {@link jakarta.ws.rs.container.ContainerRequestContext} passed to the filter | ||
* @return true to vote to start a span; false to vote not to start a span | ||
*/ | ||
boolean shouldStartSpan(ContainerRequestContext containerRequestContext); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 aProviders
to get hold of aContextResolver<...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 underMETA-INF/services
(as in the Java service loading approach). And as you pointed out earlier, usingInstance
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()
(ornext()
) 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.