-
Notifications
You must be signed in to change notification settings - Fork 129
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
Signals processing #1014
Signals processing #1014
Conversation
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.
Let's use composition here instead. Recommend:
- Rename
Interceptable
toMultiInterceptor
or something and make itfinal
It might also implementInterceptor<Collection<T>>
- Remove
extends ....
from each of the exporters. - Add constructor arg and field of type
MultiInterceptor
(or the interface) to each exporter - Call
interceptor.interceptAll()
in the export method.
.github/component_owners.yml
Outdated
@@ -47,6 +47,9 @@ components: | |||
- HaloFour | |||
noop-api: | |||
- jack-berg | |||
processors: | |||
- LikeTheSalad | |||
- ? |
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.
You can add me here. I think the literal ?
breaks things.
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.
Thanks! You're added now.
import java.util.List; | ||
import java.util.Set; | ||
|
||
/** Base class to reuse the code related to intercepting signals. */ |
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 strongly believe that we should not use object inheritance as a mechanism for reusing code.
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.
Generally speaking, I understand the benefits of composition over inheritance, although I still believe that there are cases where inheritance makes more sense. I think the problem comes when people start adding more than one responsibility to a base class. That being said though, after my recent changes to this PR I realized that it was actually better not to use inheritance for this use case 😅 so thanks for your input! Though I guess my point is that the best approach might vary depending on the use case.
|
||
/** Base class to reuse the code related to intercepting signals. */ | ||
public class Interceptable<T> { | ||
private final Set<Interceptor<T>> interceptors = new HashSet<>(); |
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 could think of a few contrived scenarios where order might matter. I'd use a list.
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.
Makes sense, I've added the changes into ComposableInterceptor.
Sounds good, I've applied the changes you mentioned with some small diffs, for the multi interceptor name I went with "ComposableInterceptor" and for the exporters' constructor arg for an interceptor I went with the interface |
processors/src/main/java/io/opentelemetry/contrib/interceptor/api/Interceptor.java
Show resolved
Hide resolved
processors/src/main/java/io/opentelemetry/contrib/interceptor/common/ComposableInterceptor.java
Outdated
Show resolved
Hide resolved
processors/src/main/java/io/opentelemetry/contrib/interceptor/common/ComposableInterceptor.java
Outdated
Show resolved
Hide resolved
...ssors/src/main/java/io/opentelemetry/contrib/interceptor/InterceptableLogRecordExporter.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/io/opentelemetry/contrib/interceptor/InterceptableLogRecordExporterTest.java
Outdated
Show resolved
Hide resolved
…api/Interceptor.java Co-authored-by: Mateusz Rzeszutek <[email protected]>
…common/ComposableInterceptor.java Co-authored-by: Mateusz Rzeszutek <[email protected]>
…common/ComposableInterceptor.java Co-authored-by: Mateusz Rzeszutek <[email protected]>
…InterceptableLogRecordExporter.java Co-authored-by: Mateusz Rzeszutek <[email protected]>
…InterceptableLogRecordExporterTest.java Co-authored-by: Mateusz Rzeszutek <[email protected]>
Thanks @mateuszrzeszutek ! I've applied your suggestions. |
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.
👍
@breedx-splk could you give this another look? thx |
Description:
Feature addition - Tools for globally processing signal data items in order to allow for:
As part of the Splunk Android SDK donation, some of the items requested from the community were related to moving generic tools used in the Splunk code over to the java-contrib repo. Some of these tools enabled filtering spans based on predefined predicates as well as adding attributes to them.
The proposed tools in this PR aren't exact replicas of the classes used in the Splunk SDK although they aim to enable the same use cases (signal modification and filtering) not only for spans but also for logs and metrics in a quite generic way that should allow for more use cases to be supported, albeit in a less verbose way, but flexible enough to be further extended to add more specific use cases on top, such as "rejectSpansNamed()" and "rejectSpansWithAttributesMatching()" for example, as some of the ones currently available in here.
There are no defined guidelines on how to accomplish this functionality, so what I'm proposing here is just a way to do so that I consider simple and flexible enough to accommodate different use cases. So I'm open to any alternatives or adjustments that could improve this feature in any way!
How it works
There's an Interceptor interface that can modify or eliminate a signal right before it gets exported. This example shows how to modify and/or filter a span out from exporting:
You can find more examples in the tests added as part of this PR.
Existing Issue(s):
Testing:
Unit tests
Documentation:
Outstanding items: