From 7e53d48320519a26c590506219706a33f0c9f8f3 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Feb 2023 14:38:54 +0100 Subject: [PATCH 1/8] Fix used instrumentations shown on shutdown --- .../apm/agent/bci/InstrumentationStats.java | 44 ++++++++++++++++++- .../agent/bci/InstrumentationStatsTest.java | 13 ++++-- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java index 2f7e415575..f2a21221a4 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java @@ -21,8 +21,11 @@ import co.elastic.apm.agent.bci.bytebuddy.MatcherTimer; import co.elastic.apm.agent.sdk.ElasticApmInstrumentation; +import java.util.ArrayList; import java.util.Collection; +import java.util.Comparator; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; @@ -58,7 +61,8 @@ public Collection getUsedInstrumentationGroups() { for (ElasticApmInstrumentation instrumentation : usedInstrumentations.keySet()) { usedInstrumentationGroups.addAll(instrumentation.getInstrumentationGroupNames()); } - for (ElasticApmInstrumentation instrumentation : allInstrumentations) { + List allDeterministic = getAllInstrumentationsSorted(); + for (ElasticApmInstrumentation instrumentation : allDeterministic) { if (usedInstrumentations.containsKey(instrumentation)) { continue; } @@ -68,10 +72,48 @@ public Collection getUsedInstrumentationGroups() { } usedInstrumentationGroups.removeAll(instrumentationGroups); } + //The loop above can accidentally disable actually used instrumentations so we add them back + for (ElasticApmInstrumentation instrumentation : allDeterministic) { + if (usedInstrumentations.containsKey(instrumentation)) { + Collection groupNames = instrumentation.getInstrumentationGroupNames(); + List withoutExperimental = new ArrayList<>(); + for (String groupName : groupNames) { + if ("experimental".equals(groupName)) { + usedInstrumentationGroups.add("experimental"); + } else { + withoutExperimental.add(groupName); + } + } + if (!withoutExperimental.isEmpty() && !containsAny(usedInstrumentationGroups, withoutExperimental)) { + //add the last group name because that is usually the most specific + usedInstrumentationGroups.add(withoutExperimental.get(withoutExperimental.size() - 1)); + } + } + } return usedInstrumentationGroups; } + private static boolean containsAny(Set set, Collection toCheck) { + for (T element : toCheck) { + if (set.contains(element)) { + return true; + } + } + return false; + } + + private List getAllInstrumentationsSorted() { + List sorted = new ArrayList<>(allInstrumentations); + sorted.sort(new Comparator() { + @Override + public int compare(ElasticApmInstrumentation o1, ElasticApmInstrumentation o2) { + return o1.getClass().getName().compareTo(o2.getClass().getName()); + } + }); + return sorted; + } + MatcherTimer getOrCreateTimer(Class adviceClass) { final String name = adviceClass.getName(); MatcherTimer timer = matcherTimers.get(name); diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java index 00706c5a7f..2f4508361a 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java @@ -33,7 +33,7 @@ class InstrumentationStatsTest { private static class NoopInstrumentation extends ElasticApmInstrumentation { - private final Collection instrumentationGroups; + private final Collection instrumentationGroups; public NoopInstrumentation(Collection instrumentationGroups) { this.instrumentationGroups = instrumentationGroups; @@ -89,14 +89,19 @@ void testUsedAndUnusedInstrumentationsWithCommonGroups() { @Test void testUsedAndUnusedInstrumentationsWithSameGroups() { NoopInstrumentation instrumentation1 = new NoopInstrumentation(Set.of("a", "b")); - NoopInstrumentation instrumentation2 = new NoopInstrumentation(Set.of("c", "d")); - NoopInstrumentation instrumentation3 = new NoopInstrumentation(Set.of("a", "b")); + NoopInstrumentation instrumentation2 = new NoopInstrumentation(Set.of("c", "experimental")); + NoopInstrumentation instrumentation3 = new NoopInstrumentation(Set.of("c", "d")); + NoopInstrumentation instrumentation4 = new NoopInstrumentation(Set.of("a", "b")); + NoopInstrumentation instrumentation5 = new NoopInstrumentation(Set.of("a", "b", "f")); instrumentationStats.addInstrumentation(instrumentation1); instrumentationStats.addInstrumentation(instrumentation2); instrumentationStats.addInstrumentation(instrumentation3); + instrumentationStats.addInstrumentation(instrumentation4); + instrumentationStats.addInstrumentation(instrumentation5); instrumentationStats.addUsedInstrumentation(instrumentation1); + instrumentationStats.addUsedInstrumentation(instrumentation2); - assertThat(instrumentationStats.getUsedInstrumentationGroups()).hasSameElementsAs(List.of("a", "b")); + assertThat(instrumentationStats.getUsedInstrumentationGroups()).hasSameElementsAs(List.of("b", "c", "experimental")); } } From 640a521f08ace3b520ce5949efa07e36250fe52a Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Feb 2023 15:25:37 +0100 Subject: [PATCH 2/8] Fix java8+ usage --- .../java/co/elastic/apm/agent/bci/InstrumentationStats.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java index f2a21221a4..78f8cdae99 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; import java.util.HashSet; import java.util.List; @@ -105,7 +106,7 @@ private static boolean containsAny(Set set, Collection toCheck) { private List getAllInstrumentationsSorted() { List sorted = new ArrayList<>(allInstrumentations); - sorted.sort(new Comparator() { + Collections.sort(sorted, new Comparator() { @Override public int compare(ElasticApmInstrumentation o1, ElasticApmInstrumentation o2) { return o1.getClass().getName().compareTo(o2.getClass().getName()); From 0148013abfe00fc0d309018dfee3599d2f15742e Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Feb 2023 16:02:07 +0100 Subject: [PATCH 3/8] Fixed test indeterminism --- .../apm/agent/bci/InstrumentationStatsTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java index 2f4508361a..c08a63ceb9 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java @@ -88,11 +88,12 @@ void testUsedAndUnusedInstrumentationsWithCommonGroups() { @Test void testUsedAndUnusedInstrumentationsWithSameGroups() { - NoopInstrumentation instrumentation1 = new NoopInstrumentation(Set.of("a", "b")); - NoopInstrumentation instrumentation2 = new NoopInstrumentation(Set.of("c", "experimental")); - NoopInstrumentation instrumentation3 = new NoopInstrumentation(Set.of("c", "d")); - NoopInstrumentation instrumentation4 = new NoopInstrumentation(Set.of("a", "b")); - NoopInstrumentation instrumentation5 = new NoopInstrumentation(Set.of("a", "b", "f")); + //Anonymous classes to make the order deterministic + NoopInstrumentation instrumentation1 = new NoopInstrumentation(Set.of("a", "b")){}; + NoopInstrumentation instrumentation2 = new NoopInstrumentation(Set.of("c", "experimental")){}; + NoopInstrumentation instrumentation3 = new NoopInstrumentation(Set.of("c", "d")){}; + NoopInstrumentation instrumentation4 = new NoopInstrumentation(Set.of("a", "b")){}; + NoopInstrumentation instrumentation5 = new NoopInstrumentation(Set.of("a", "b", "f")){}; instrumentationStats.addInstrumentation(instrumentation1); instrumentationStats.addInstrumentation(instrumentation2); @@ -102,6 +103,6 @@ void testUsedAndUnusedInstrumentationsWithSameGroups() { instrumentationStats.addUsedInstrumentation(instrumentation1); instrumentationStats.addUsedInstrumentation(instrumentation2); - assertThat(instrumentationStats.getUsedInstrumentationGroups()).hasSameElementsAs(List.of("b", "c", "experimental")); + assertThat(instrumentationStats.getUsedInstrumentationGroups()).hasSameElementsAs(List.of("a", "c", "experimental")); } } From ee0f9c9249163186077d5d163528e25a4ba44aa5 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Feb 2023 16:11:36 +0100 Subject: [PATCH 4/8] Added changelog --- CHANGELOG.asciidoc | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 8b1871b529..2582cc8b47 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -20,20 +20,6 @@ endif::[] === Unreleased -[[release-notes-x.x.x]] -==== x.x.x - YYYY/MM/DD - -[float] -===== Breaking changes - -[float] -===== Features - -* Cool new feature: {pull}2526[#2526] - -[float] -===== Bug fixes - [[release-notes-1.37.0]] ==== 1.37.0 - YYYY/MM/DD @@ -44,6 +30,7 @@ communication - {pull}2996[#2996] [float] ===== Bug fixes +* Fixed used instrumentations printed on shutdown {pull}3001[#3001] [[release-notes-1.x]] === Java Agent version 1.x From abe577c5d2ec12696f009f5a4339bdd1811180af Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Feb 2023 17:36:27 +0100 Subject: [PATCH 5/8] Next attempt to fix tests --- .../agent/bci/InstrumentationStatsTest.java | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java index c08a63ceb9..e8a92ab240 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java @@ -89,11 +89,31 @@ void testUsedAndUnusedInstrumentationsWithCommonGroups() { @Test void testUsedAndUnusedInstrumentationsWithSameGroups() { //Anonymous classes to make the order deterministic - NoopInstrumentation instrumentation1 = new NoopInstrumentation(Set.of("a", "b")){}; - NoopInstrumentation instrumentation2 = new NoopInstrumentation(Set.of("c", "experimental")){}; - NoopInstrumentation instrumentation3 = new NoopInstrumentation(Set.of("c", "d")){}; - NoopInstrumentation instrumentation4 = new NoopInstrumentation(Set.of("a", "b")){}; - NoopInstrumentation instrumentation5 = new NoopInstrumentation(Set.of("a", "b", "f")){}; + NoopInstrumentation instrumentation1 = new NoopInstrumentation(Set.of("a", "b")) { + public String toString() { + return "clazz1"; + } + }; + NoopInstrumentation instrumentation2 = new NoopInstrumentation(Set.of("c", "experimental")) { + public String toString() { + return "clazz2"; + } + }; + NoopInstrumentation instrumentation3 = new NoopInstrumentation(Set.of("c", "d")) { + public String toString() { + return "clazz3"; + } + }; + NoopInstrumentation instrumentation4 = new NoopInstrumentation(Set.of("a", "b")) { + public String toString() { + return "clazz4"; + } + }; + NoopInstrumentation instrumentation5 = new NoopInstrumentation(Set.of("a", "b", "f")) { + public String toString() { + return "clazz5"; + } + }; instrumentationStats.addInstrumentation(instrumentation1); instrumentationStats.addInstrumentation(instrumentation2); @@ -103,6 +123,6 @@ void testUsedAndUnusedInstrumentationsWithSameGroups() { instrumentationStats.addUsedInstrumentation(instrumentation1); instrumentationStats.addUsedInstrumentation(instrumentation2); - assertThat(instrumentationStats.getUsedInstrumentationGroups()).hasSameElementsAs(List.of("a", "c", "experimental")); + assertThat(instrumentationStats.getUsedInstrumentationGroups()).hasSameElementsAs(List.of("b", "c", "experimental")); } } From 9e0b7c5b96113d8e2bc54fc57fafe6d7134fdc47 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Feb 2023 17:57:58 +0100 Subject: [PATCH 6/8] Aaaaaand next attempt --- .../agent/bci/InstrumentationStatsTest.java | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java index e8a92ab240..f8d3a8f0fb 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationStatsTest.java @@ -89,30 +89,15 @@ void testUsedAndUnusedInstrumentationsWithCommonGroups() { @Test void testUsedAndUnusedInstrumentationsWithSameGroups() { //Anonymous classes to make the order deterministic - NoopInstrumentation instrumentation1 = new NoopInstrumentation(Set.of("a", "b")) { - public String toString() { - return "clazz1"; - } + NoopInstrumentation instrumentation1 = new NoopInstrumentation(List.of("a", "b")) { }; - NoopInstrumentation instrumentation2 = new NoopInstrumentation(Set.of("c", "experimental")) { - public String toString() { - return "clazz2"; - } + NoopInstrumentation instrumentation2 = new NoopInstrumentation(List.of("c", "experimental")) { }; - NoopInstrumentation instrumentation3 = new NoopInstrumentation(Set.of("c", "d")) { - public String toString() { - return "clazz3"; - } + NoopInstrumentation instrumentation3 = new NoopInstrumentation(List.of("c", "d")) { }; - NoopInstrumentation instrumentation4 = new NoopInstrumentation(Set.of("a", "b")) { - public String toString() { - return "clazz4"; - } + NoopInstrumentation instrumentation4 = new NoopInstrumentation(List.of("a", "b")) { }; - NoopInstrumentation instrumentation5 = new NoopInstrumentation(Set.of("a", "b", "f")) { - public String toString() { - return "clazz5"; - } + NoopInstrumentation instrumentation5 = new NoopInstrumentation(List.of("a", "b", "f")) { }; instrumentationStats.addInstrumentation(instrumentation1); From 4c97123dd30523c678f46f93a8809510c1691f4b Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Mon, 6 Feb 2023 09:29:10 +0100 Subject: [PATCH 7/8] Added sysout to understand what's going on --- .../java/co/elastic/apm/agent/bci/InstrumentationStats.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java index 78f8cdae99..03fa93d87c 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java @@ -64,6 +64,7 @@ public Collection getUsedInstrumentationGroups() { } List allDeterministic = getAllInstrumentationsSorted(); for (ElasticApmInstrumentation instrumentation : allDeterministic) { + System.out.println("Instrumentation in order: " + instrumentation.getClass().getName()); if (usedInstrumentations.containsKey(instrumentation)) { continue; } @@ -109,7 +110,9 @@ private List getAllInstrumentationsSorted() { Collections.sort(sorted, new Comparator() { @Override public int compare(ElasticApmInstrumentation o1, ElasticApmInstrumentation o2) { - return o1.getClass().getName().compareTo(o2.getClass().getName()); + String name1 = o1.getClass().getName(); + String name2 = o2.getClass().getName(); + return name1.compareTo(name2); } }); return sorted; From 7bf98fab6d223c01745b4dc1b93fb18c776f4326 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Mon, 6 Feb 2023 10:11:20 +0100 Subject: [PATCH 8/8] Removed sysout again --- .../main/java/co/elastic/apm/agent/bci/InstrumentationStats.java | 1 - 1 file changed, 1 deletion(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java index 03fa93d87c..22be3983f4 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/InstrumentationStats.java @@ -64,7 +64,6 @@ public Collection getUsedInstrumentationGroups() { } List allDeterministic = getAllInstrumentationsSorted(); for (ElasticApmInstrumentation instrumentation : allDeterministic) { - System.out.println("Instrumentation in order: " + instrumentation.getClass().getName()); if (usedInstrumentations.containsKey(instrumentation)) { continue; }