Introduce IgnoredTypesConfigurer SPI to enable defining per-module ignores #3219

mateuszrzeszutek · 2021-06-07T18:03:26Z

When I was writing the spring-integration javaagent instrumentation I had to add yet another entry (a few of them, in fact) into the AdditionalLibraryIgnoresMatcher - an I thought that maybe it's a good idea to finally make the first step towards splitting that class and keeping the lib-specific ignores in the lib instrumentation itself.

This PR introduces a new IgnoredTypesConfigurer SPI that allows adding ignored/allowed classes to the agent builder in any instrumentation module (or extension jar). All those ignores are still kept in one place, but now it's possible to split that one giant class and move some of those overrides to related instrumentation modules. I still don't have a good idea what to do about our tests that verify that classes matched by AdditionalLibraryIgnoresMatcher were not instrumented, but maybe it'll become clear with the first instrumentation.

The same SPI could be used to exclude task classes (ExecutorInstrumentationUtils) from context propagation, although it's not implemented yet.

The new SPI stores all those overrides in a prefix tree, which has a bit better performance than repeated .startsWith calls:

The old version (.startsWith(...)):

Benchmark                                                                              Mode  Cnt         Score       Error   Units
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType                         thrpt   10         9.744 ±     0.428  ops/us
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType:heap.total.after        thrpt   10  41943040.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType:heap.total.before       thrpt   10  41943040.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType:heap.used.after         thrpt   10  10787619.200 ± 72013.009   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType:heap.used.before        thrpt   10  10649194.400 ± 56761.205   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType:·gc.alloc.rate          thrpt   10        ≈ 10⁻⁴              MB/sec
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType:·gc.alloc.rate.norm     thrpt   10        ≈ 10⁻⁴                B/op
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_appType:·gc.count               thrpt   10           ≈ 0              counts
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType                      thrpt   10         3.172 ±     0.045  ops/us
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType:heap.total.after     thrpt   10  41943040.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType:heap.total.before    thrpt   10  41943040.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType:heap.used.after      thrpt   10  10748656.000 ± 77617.852   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType:heap.used.before     thrpt   10  10652167.200 ± 56637.563   bytes
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType:·gc.alloc.rate       thrpt   10        ≈ 10⁻⁴              MB/sec
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType:·gc.alloc.rate.norm  thrpt   10        ≈ 10⁻⁴                B/op
IgnoredTypesMatcherBenchmark.additionalLibraryMatcher_springType:·gc.count            thrpt   10           ≈ 0              counts

The new version (trie):

Benchmark                                                                              Mode  Cnt         Score       Error   Units
IgnoredTypesMatcherBenchmark.trieMatcher_appType                                      thrpt   10        38.168 ±     2.112  ops/us
IgnoredTypesMatcherBenchmark.trieMatcher_appType:heap.total.after                     thrpt   10  35651584.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_appType:heap.total.before                    thrpt   10  35651584.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_appType:heap.used.after                      thrpt   10  10688258.400 ± 59034.737   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_appType:heap.used.before                     thrpt   10  10607850.400 ± 47090.969   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_appType:·gc.alloc.rate                       thrpt   10        ≈ 10⁻⁴              MB/sec
IgnoredTypesMatcherBenchmark.trieMatcher_appType:·gc.alloc.rate.norm                  thrpt   10        ≈ 10⁻⁵                B/op
IgnoredTypesMatcherBenchmark.trieMatcher_appType:·gc.count                            thrpt   10           ≈ 0              counts
IgnoredTypesMatcherBenchmark.trieMatcher_springType                                   thrpt   10         9.597 ±     0.330  ops/us
IgnoredTypesMatcherBenchmark.trieMatcher_springType:heap.total.after                  thrpt   10  41943040.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_springType:heap.total.before                 thrpt   10  41943040.000 ±     0.001   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_springType:heap.used.after                   thrpt   10  10749009.600 ± 71859.031   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_springType:heap.used.before                  thrpt   10  10652520.800 ± 56659.667   bytes
IgnoredTypesMatcherBenchmark.trieMatcher_springType:·gc.alloc.rate                    thrpt   10        ≈ 10⁻⁴              MB/sec
IgnoredTypesMatcherBenchmark.trieMatcher_springType:·gc.alloc.rate.norm               thrpt   10        ≈ 10⁻⁴                B/op
IgnoredTypesMatcherBenchmark.trieMatcher_springType:·gc.count                         thrpt   10           ≈ 0              counts

I didn't really trust my algo skills, fortunately this benchmark proved that I didn't make matters worse with this change 😂

…nores

trask

nice 👍

...nsion-api/src/main/java/io/opentelemetry/javaagent/extension/ignore/IgnoredTypesBuilder.java

trask · 2021-06-08T03:41:29Z

javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/IgnoreMatcherProvider.java

+@Deprecated
 public interface IgnoreMatcherProvider {


always nice to remove something when adding something 😄👍

...tooling/src/main/java/io/opentelemetry/javaagent/tooling/ignore/IgnoredTypesBuilderImpl.java

benchmark/src/jmh/java/io/opentelemetry/benchmark/IgnoredTypesMatcherBenchmark.java

...nsion-api/src/main/java/io/opentelemetry/javaagent/extension/ignore/IgnoredTypesBuilder.java

...on-api/src/main/java/io/opentelemetry/javaagent/extension/ignore/IgnoredTypesConfigurer.java

anuraaga · 2021-06-10T00:59:24Z

...on-api/src/main/java/io/opentelemetry/javaagent/extension/ignore/IgnoredTypesConfigurer.java

+ * <p>This is a service provider interface that requires implementations to be registered in a
+ * provider-configuration file stored in the {@code META-INF/services} resource directory.
+ */
+public interface IgnoredTypesConfigurer extends Ordered {


Is there a reason for a separate interface instead of knob on InstrumentationModule?

I thought that it'd be useful to be able ignore some classes (e.g. task classes to prevent context leaking) even if you turn the instrumentation off.

Hmm - well we could choose to respet them even if it's disabled? Not sure if it's a great idea - IIRC, dd-trace-java has the knob on InstrumentationModule so it could keep us more consistent with them, though maybe that's a difficult goal at this point

As far as I understand, they have something similar just for concurrent/task classes: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ExcludeFilterProvider.java
And they're doing a separate pass over Instrumenters and check with instanceof if this interface is implemented.

it could keep us more consistent with them, though maybe that's a difficult goal at this point

With all our changes to muzzle, the split to InstrumentationModule/TypeInstrumentation, and DD's custom things (like profiling) it's probably impossible 😅

Hmm - well we could choose to respet them even if it's disabled?

That's what seemed a bit confusing to me - you have the enabled flag but it only works for some InstrumentationModule methods.

anuraaga · 2021-06-11T07:31:50Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ignore/trie/Trie.java

+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+/** A prefix tree that maps from the longest matching prefix to a value {@code V}. */


Did you copy this class from anywhere?

No, I wrote it from scratch.

anuraaga · 2021-06-11T07:43:16Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ignore/trie/TrieImpl.java

+
+    @Nullable
+    Node<V> getNext(char c) {
+      int index = Arrays.binarySearch(chars, c);


If you feel like comparing some optimizations, some ideas are

Don't binary search, these arrays seem small enough that it would be slower than a normal loop

A single array with every other element Character and Node - single array means better cache performance

Copy in from Armeria (while simplifying it's funcionality for parameters) :P

https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/server/RoutingTrieBuilder.java

The main difference is the nodes are subpaths rather than characters - it's quite unfortunate a boring path like org.springframework.blah has a node per character

Nice, thanks!
I'll note all your suggestions down and try them out later.

…nores (open-telemetry#3219)

Introduce IgnoredTypesConfigurer SPI to enable defining per-module ig…

52d7eed

…nores

mateuszrzeszutek requested review from anuraaga, iNikem, jkwatson, laurit, pavolloffay, trask and tylerbenson as code owners June 7, 2021 18:03

trask approved these changes Jun 8, 2021

View reviewed changes

code review comments

1f4f91b

iNikem reviewed Jun 9, 2021

View reviewed changes

anuraaga reviewed Jun 10, 2021

View reviewed changes

Mateusz Rzeszutek added 2 commits June 10, 2021 10:29

code review comments

13a2cd0

spring-webflux latestDepTest fix

4ee8d8e

anuraaga reviewed Jun 11, 2021

View reviewed changes

anuraaga approved these changes Jun 11, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into ignored-types

136c50e

trask merged commit 3b09d46 into open-telemetry:main Jun 11, 2021

mateuszrzeszutek deleted the ignored-types branch June 12, 2021 08:38

mateuszrzeszutek mentioned this pull request Jun 14, 2021

Rewrite global ignores matcher to IgnoredTypesConfigurer #3298

Merged

robododge pushed a commit to robododge/opentelemetry-java-instrumentation that referenced this pull request Jun 17, 2021

Introduce IgnoredTypesConfigurer SPI to enable defining per-module ig…

190294b

…nores (open-telemetry#3219)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce IgnoredTypesConfigurer SPI to enable defining per-module ignores #3219

Introduce IgnoredTypesConfigurer SPI to enable defining per-module ignores #3219

mateuszrzeszutek commented Jun 7, 2021

trask left a comment

trask Jun 8, 2021

anuraaga Jun 10, 2021

mateuszrzeszutek Jun 10, 2021

anuraaga Jun 10, 2021

mateuszrzeszutek Jun 11, 2021

anuraaga Jun 11, 2021

mateuszrzeszutek Jun 11, 2021

anuraaga Jun 11, 2021

mateuszrzeszutek Jun 11, 2021


		import org.checkerframework.checker.nullness.qual.Nullable;

		/** A prefix tree that maps from the longest matching prefix to a value {@code V}. */

Introduce IgnoredTypesConfigurer SPI to enable defining per-module ignores #3219

Introduce IgnoredTypesConfigurer SPI to enable defining per-module ignores #3219

Conversation

mateuszrzeszutek commented Jun 7, 2021

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment