Skip to content
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

Merged
merged 20 commits into from
Sep 7, 2023
Merged

Conversation

LikeTheSalad
Copy link
Contributor

@LikeTheSalad LikeTheSalad commented Aug 23, 2023

Description:

Feature addition - Tools for globally processing signal data items in order to allow for:

  • Modification
  • Filtering

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:

    InMemorySpanExporter myExporter = InMemorySpanExporter.create();
    ComposableInterceptor interceptor = new ComposableInterceptor();
    Tracer tracer = createTracerWithExporter(new InterceptableSpanExporter(myExporter, interceptor));

    // This interceptor will modify all the spans by adding a new attr.
    interceptor.add(
        spanData -> {
          ModifiableSpanData modified = new ModifiableSpanData(spanData);
          modified.attributes.put("global.attr", "from interceptor");
          return modified;
        });

    // This interceptor will prevent some spans from getting exported:
    interceptor.add(
        spanData -> {
          if (spanData.getName().endsWith("deleted")) {
            return null;
          }
          return spanData;
        });

     // All the spans will go through both interceptors when getting exported
    tracer.spanBuilder("My Span").startSpan()//etc...

You can find more examples in the tests added as part of this PR.

Existing Issue(s):

Testing:

Unit tests

Documentation:

Outstanding items:

@LikeTheSalad LikeTheSalad requested a review from a team August 23, 2023 16:19
Copy link
Contributor

@breedx-splk breedx-splk left a 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 to MultiInterceptor or something and make it final It might also implement Interceptor<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.

@@ -47,6 +47,9 @@ components:
- HaloFour
noop-api:
- jack-berg
processors:
- LikeTheSalad
- ?
Copy link
Contributor

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.

Copy link
Contributor Author

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. */
Copy link
Contributor

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.

Copy link
Contributor Author

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<>();
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@LikeTheSalad
Copy link
Contributor Author

Let's use composition here instead. Recommend:

Rename Interceptable to MultiInterceptor or something and make it final It might also implement Interceptor<Collection>
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.

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 Interceptor instead of any implementation to allow users to use any kind of implementation they'd like. Also, the interceptAll method was made a default Interceptor method since it seems appropriate as a reusable functionality regardless of the implementation.

@LikeTheSalad
Copy link
Contributor Author

Thanks @mateuszrzeszutek ! I've applied your suggestions.

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@trask
Copy link
Member

trask commented Sep 5, 2023

@breedx-splk could you give this another look? thx

@trask trask merged commit d56ea37 into open-telemetry:main Sep 7, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants