-
Notifications
You must be signed in to change notification settings - Fork 4k
opentelemetry: Add target attribute filter for metrics #12587
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
base: master
Are you sure you want to change the base?
Conversation
Introduce an optional Predicate<String> targetAttributeFilter in the OpenTelemetryMetricsModule to control how grpc.target is recorded in metrics. Also add support in GrpcOpenTelemetry.Builder to allow users to configure this filter when building the module.
|
Thanks for looping me in. I'll keep an eye on the Android desugaring discussion. In the meantime, I've updated the PR and gRFC A109 with a null-safety check for the filtering logic to make it more robust. If it turns out that using Java 8's Predicate causes compatibility concerns, I'm happy to adjust the interface accordingly (e.g., using a small custom functional interface), based on what the maintainers think is best. |
| * | ||
| * <p>If unset, all targets are recorded as-is. | ||
| */ | ||
| public Builder targetAttributeFilter(@Nullable Predicate<String> filter) { |
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.
This does need @ExperimentalApi. Go ahead and create an issue to use with it.
| private final Map<String, Boolean> enableMetrics = new HashMap<>(); | ||
| private boolean disableAll; | ||
| @Nullable | ||
| private Predicate<String> targetAttributeFilter; |
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.
So #12592 does impact this. I had meant to send out a review on Tuesday, but failed to figure out the testing implication. I still haven't figured out the testing implication...
The targetAttributeFilter() API itself is fine. That API doesn't seem too important to call from Android, and most people probably are using API desugaring anyway. You can annotate it with @IgnoreJRERequirement. However, this field would be a problem. Let's make our own interface instead of Predicate to store the value here. You'd convert from Predicate to our interface within the targetAttributeFilter() method. Android users that don't call targetAttributeFilter() then have no concerns about Predicate being available, because we aren't using it in any code they call. That gives us a nice API and just complicates our internals slightly until we drop support for Android API level 23.
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.
Thank you for your detailed feedback and guidance on this PR. I have updated the code to address your concerns regarding Android compatibility and project conventions.
- Internal Interface: I introduced a package-private
TargetFilternested interface withinGrpcOpenTelemetry. By replacingPredicatein the class fields with this interface, the module remains safe to load in older Android environments (API < 24) when the filtering feature is not used. - Code Consistency: To be extra cautious, I have avoided using lambdas for the internal filter implementations where possible, ensuring maximum compatibility even in the test code.
- Experimental API: I've marked the new builder method with
@ExperimentalApiand linked it to the tracking issue. - Android CI Compatibility: As you suggested, I have annotated the filter method with @IgnoreJRERequirement to resolve the Animal Sniffer CI failures. I also updated the build.gradle for the opentelemetry module to include the necessary animalsniffer annotations dependency.
I have also updated the corresponding gRFC (A109) to reflect these implementation details, ensuring the proposal stays in sync with the actual Java implementation.
Tracking Issue: #12595
Please let me know if there are any further adjustments needed.
Replace Predicate with a package-private TargetFilter interface in fields to ensure compatibility with Android API levels < 24. This prevents potential runtime issues for Android users who do not use the filtering feature. Mark the API as experimental and link the tracking issue.
Introduce an optional Predicate targetAttributeFilter to control how grpc.target is recorded in OpenTelemetry client metrics.
When a filter is provided, targets rejected by the predicate are normalized to "other" to reduce grpc.target metric cardinality, while accepted targets are recorded as-is. If no filter is set, existing behavior is preserved.
This change adds a new Builder API on GrpcOpenTelemetry to allow applications to configure the filter. Tests verify both the Builder wiring and the target normalization behavior.
This is an optional API; annotation (e.g., experimental) can be added per maintainer guidance.
Refs #12322
Related: gRFC A109 – Target Attribute Filter for OpenTelemetry Metrics
grpc/proposal#528