Use separate TestEventHandlers per framework in CI Vis instrumentations #8451
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daniel-mohedano
added
type: refactoring
comp: ci visibility
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 4 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.037 s) : 0, 1037394
Total [baseline] (10.368 s) : 0, 10368350
Agent [candidate] (1.039 s) : 0, 1038873
Total [candidate] (10.407 s) : 0, 10406687
section appsec
Agent [baseline] (1.18 s) : 0, 1180388
Total [baseline] (10.73 s) : 0, 10730272
Agent [candidate] (1.181 s) : 0, 1181410
Total [candidate] (10.744 s) : 0, 10744129
section iast
Agent [baseline] (1.171 s) : 0, 1170838
Total [baseline] (10.92 s) : 0, 10919816
Agent [candidate] (1.171 s) : 0, 1170918
Total [candidate] (10.92 s) : 0, 10919672
section profiling
Agent [baseline] (1.255 s) : 0, 1255335
Total [baseline] (10.833 s) : 0, 10833213
Agent [candidate] (1.259 s) : 0, 1259230
Total [candidate] (10.81 s) : 0, 10809502
gantt
title petclinic - break down per module: candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (715.389 ms) : 0, 715389
BytebuddyAgent [candidate] (717.146 ms) : 0, 717146
GlobalTracer [baseline] (238.205 ms) : 0, 238205
GlobalTracer [candidate] (238.65 ms) : 0, 238650
AppSec [baseline] (55.505 ms) : 0, 55505
AppSec [candidate] (55.459 ms) : 0, 55459
Remote Config [baseline] (681.314 µs) : 0, 681
Remote Config [candidate] (684.072 µs) : 0, 684
Telemetry [baseline] (12.723 ms) : 0, 12723
Telemetry [candidate] (12.041 ms) : 0, 12041
section appsec
BytebuddyAgent [baseline] (734.314 ms) : 0, 734314
BytebuddyAgent [candidate] (734.033 ms) : 0, 734033
GlobalTracer [baseline] (234.94 ms) : 0, 234940
GlobalTracer [candidate] (235.494 ms) : 0, 235494
AppSec [baseline] (176.59 ms) : 0, 176590
AppSec [candidate] (177.149 ms) : 0, 177149
Remote Config [baseline] (652.382 µs) : 0, 652
Remote Config [candidate] (668.533 µs) : 0, 669
Telemetry [baseline] (8.257 ms) : 0, 8257
Telemetry [candidate] (8.328 ms) : 0, 8328
IAST [baseline] (21.461 ms) : 0, 21461
IAST [candidate] (21.714 ms) : 0, 21714
section iast
BytebuddyAgent [baseline] (836.323 ms) : 0, 836323
BytebuddyAgent [candidate] (836.868 ms) : 0, 836868
GlobalTracer [baseline] (230.092 ms) : 0, 230092
GlobalTracer [candidate] (229.811 ms) : 0, 229811
AppSec [baseline] (57.178 ms) : 0, 57178
AppSec [candidate] (57.12 ms) : 0, 57120
Remote Config [baseline] (606.899 µs) : 0, 607
Remote Config [candidate] (604.737 µs) : 0, 605
Telemetry [baseline] (8.758 ms) : 0, 8758
Telemetry [candidate] (8.716 ms) : 0, 8716
IAST [baseline] (22.945 ms) : 0, 22945
IAST [candidate] (22.844 ms) : 0, 22844
section profiling
ProfilingAgent [baseline] (95.856 ms) : 0, 95856
ProfilingAgent [candidate] (95.238 ms) : 0, 95238
BytebuddyAgent [baseline] (706.781 ms) : 0, 706781
BytebuddyAgent [candidate] (708.711 ms) : 0, 708711
GlobalTracer [baseline] (347.837 ms) : 0, 347837
GlobalTracer [candidate] (350.568 ms) : 0, 350568
AppSec [baseline] (54.962 ms) : 0, 54962
AppSec [candidate] (54.52 ms) : 0, 54520
Remote Config [baseline] (678.22 µs) : 0, 678
Remote Config [candidate] (676.109 µs) : 0, 676
Telemetry [baseline] (9.006 ms) : 0, 9006
Telemetry [candidate] (9.043 ms) : 0, 9043
Profiling [baseline] (95.881 ms) : 0, 95881
Profiling [candidate] (95.262 ms) : 0, 95262
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.042 s) : 0, 1041889
Total [baseline] (8.658 s) : 0, 8657704
Agent [candidate] (1.037 s) : 0, 1037429
Total [candidate] (8.633 s) : 0, 8633305
section iast
Agent [baseline] (1.168 s) : 0, 1168374
Total [baseline] (9.288 s) : 0, 9288472
Agent [candidate] (1.168 s) : 0, 1167914
Total [candidate] (9.242 s) : 0, 9241727
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.169 s) : 0, 1168834
Total [baseline] (9.178 s) : 0, 9177562
Agent [candidate] (1.177 s) : 0, 1177160
Total [candidate] (9.188 s) : 0, 9188242
section iast_TELEMETRY_OFF
Agent [baseline] (1.168 s) : 0, 1167614
Total [baseline] (9.205 s) : 0, 9204523
Agent [candidate] (1.174 s) : 0, 1173910
Total [candidate] (9.214 s) : 0, 9213536
gantt
title insecure-bank - break down per module: candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (718.771 ms) : 0, 718771
BytebuddyAgent [candidate] (715.843 ms) : 0, 715843
GlobalTracer [baseline] (238.938 ms) : 0, 238938
GlobalTracer [candidate] (238.614 ms) : 0, 238614
AppSec [baseline] (55.647 ms) : 0, 55647
AppSec [candidate] (55.34 ms) : 0, 55340
Remote Config [baseline] (698.142 µs) : 0, 698
Remote Config [candidate] (690.384 µs) : 0, 690
Telemetry [baseline] (12.929 ms) : 0, 12929
Telemetry [candidate] (12.075 ms) : 0, 12075
section iast
BytebuddyAgent [baseline] (835.432 ms) : 0, 835432
BytebuddyAgent [candidate] (835.092 ms) : 0, 835092
GlobalTracer [baseline] (229.191 ms) : 0, 229191
GlobalTracer [candidate] (229.076 ms) : 0, 229076
IAST [baseline] (22.731 ms) : 0, 22731
IAST [candidate] (22.54 ms) : 0, 22540
AppSec [baseline] (56.769 ms) : 0, 56769
AppSec [candidate] (56.999 ms) : 0, 56999
Remote Config [baseline] (619.971 µs) : 0, 620
Remote Config [candidate] (615.267 µs) : 0, 615
Telemetry [baseline] (8.684 ms) : 0, 8684
Telemetry [candidate] (8.614 ms) : 0, 8614
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (835.377 ms) : 0, 835377
BytebuddyAgent [candidate] (841.766 ms) : 0, 841766
GlobalTracer [baseline] (229.229 ms) : 0, 229229
GlobalTracer [candidate] (230.481 ms) : 0, 230481
IAST [baseline] (23.066 ms) : 0, 23066
IAST [candidate] (23.097 ms) : 0, 23097
AppSec [baseline] (56.898 ms) : 0, 56898
AppSec [candidate] (57.278 ms) : 0, 57278
Remote Config [baseline] (607.521 µs) : 0, 608
Remote Config [candidate] (621.098 µs) : 0, 621
Telemetry [baseline] (8.737 ms) : 0, 8737
Telemetry [candidate] (8.785 ms) : 0, 8785
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (834.914 ms) : 0, 834914
BytebuddyAgent [candidate] (839.364 ms) : 0, 839364
GlobalTracer [baseline] (229.88 ms) : 0, 229880
GlobalTracer [candidate] (230.757 ms) : 0, 230757
IAST [baseline] (25.236 ms) : 0, 25236
IAST [candidate] (25.575 ms) : 0, 25575
AppSec [baseline] (53.474 ms) : 0, 53474
AppSec [candidate] (53.963 ms) : 0, 53963
Remote Config [baseline] (604.506 µs) : 0, 605
Remote Config [candidate] (622.08 µs) : 0, 622
Telemetry [baseline] (8.541 ms) : 0, 8541
Telemetry [candidate] (8.644 ms) : 0, 8644
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 19 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section baseline
no_agent (371.209 µs) : 351, 391
. : milestone, 371,
iast (505.056 µs) : 483, 527
. : milestone, 505,
iast_FULL (726.929 µs) : 705, 749
. : milestone, 727,
iast_GLOBAL (550.791 µs) : 529, 573
. : milestone, 551,
iast_HARDCODED_SECRET_DISABLED (502.276 µs) : 480, 524
. : milestone, 502,
iast_INACTIVE (460.743 µs) : 439, 482
. : milestone, 461,
iast_TELEMETRY_OFF (497.349 µs) : 474, 521
. : milestone, 497,
tracing (448.901 µs) : 428, 470
. : milestone, 449,
section candidate
no_agent (376.237 µs) : 356, 396
. : milestone, 376,
iast (501.133 µs) : 479, 524
. : milestone, 501,
iast_FULL (724.582 µs) : 703, 746
. : milestone, 725,
iast_GLOBAL (555.159 µs) : 533, 578
. : milestone, 555,
iast_HARDCODED_SECRET_DISABLED (511.476 µs) : 489, 534
. : milestone, 511,
iast_INACTIVE (454.276 µs) : 432, 476
. : milestone, 454,
iast_TELEMETRY_OFF (496.761 µs) : 473, 520
. : milestone, 497,
tracing (451.984 µs) : 431, 473
. : milestone, 452,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section baseline
no_agent (1.35 ms) : 1329, 1370
. : milestone, 1350,
appsec (1.743 ms) : 1719, 1767
. : milestone, 1743,
appsec_no_iast (1.736 ms) : 1712, 1759
. : milestone, 1736,
code_origins (1.702 ms) : 1670, 1735
. : milestone, 1702,
iast (1.501 ms) : 1476, 1526
. : milestone, 1501,
profiling (1.508 ms) : 1483, 1533
. : milestone, 1508,
tracing (1.483 ms) : 1458, 1508
. : milestone, 1483,
section candidate
no_agent (1.358 ms) : 1339, 1377
. : milestone, 1358,
appsec (1.751 ms) : 1728, 1774
. : milestone, 1751,
appsec_no_iast (1.727 ms) : 1703, 1751
. : milestone, 1727,
code_origins (1.679 ms) : 1646, 1712
. : milestone, 1679,
iast (1.512 ms) : 1487, 1536
. : milestone, 1512,
profiling (1.508 ms) : 1483, 1533
. : milestone, 1508,
tracing (1.48 ms) : 1455, 1506
. : milestone, 1480,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section baseline
no_agent (1.485 ms) : 1474, 1497
. : milestone, 1485,
appsec (2.352 ms) : 2309, 2395
. : milestone, 2352,
iast (2.118 ms) : 2062, 2173
. : milestone, 2118,
iast_GLOBAL (2.171 ms) : 2116, 2227
. : milestone, 2171,
profiling (2.0 ms) : 1955, 2046
. : milestone, 2000,
tracing (1.966 ms) : 1924, 2009
. : milestone, 1966,
section candidate
no_agent (1.48 ms) : 1469, 1492
. : milestone, 1480,
appsec (2.35 ms) : 2307, 2394
. : milestone, 2350,
iast (2.131 ms) : 2076, 2186
. : milestone, 2131,
iast_GLOBAL (2.162 ms) : 2107, 2218
. : milestone, 2162,
profiling (1.98 ms) : 1936, 2025
. : milestone, 1980,
tracing (1.944 ms) : 1902, 1986
. : milestone, 1944,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.47.0-SNAPSHOT~d4a3015871, baseline=1.47.0-SNAPSHOT~93bbda11f7
dateFormat X
axisFormat %s
section baseline
no_agent (15.566 s) : 15566000, 15566000
. : milestone, 15566000,
appsec (15.019 s) : 15019000, 15019000
. : milestone, 15019000,
iast (19.097 s) : 19097000, 19097000
. : milestone, 19097000,
iast_GLOBAL (18.045 s) : 18045000, 18045000
. : milestone, 18045000,
profiling (15.143 s) : 15143000, 15143000
. : milestone, 15143000,
tracing (14.868 s) : 14868000, 14868000
. : milestone, 14868000,
section candidate
no_agent (15.022 s) : 15022000, 15022000
. : milestone, 15022000,
appsec (15.099 s) : 15099000, 15099000
. : milestone, 15099000,
iast (19.242 s) : 19242000, 19242000
. : milestone, 19242000,
iast_GLOBAL (18.104 s) : 18104000, 18104000
. : milestone, 18104000,
profiling (15.479 s) : 15479000, 15479000
. : milestone, 15479000,
tracing (14.748 s) : 14748000, 14748000
. : milestone, 14748000,
|
| } | ||
|
|
||
| public static void stop(TestFrameworkInstrumentation framework) { | ||
| TestEventsHandler<TestSuiteDescriptor, TestDescriptor> handler = HANDLERS.get(framework); |
There was a problem hiding this comment.
You can try removing right away, then check for null and close if there was an actual entry:
Suggested change
| TestEventsHandler<TestSuiteDescriptor, TestDescriptor> handler = HANDLERS.get(framework); | |
| TestEventsHandler<TestSuiteDescriptor, TestDescriptor> handler = HANDLERS.remove(framework); |
That way we only have to do one map lookup
| // store one handler per framework running | ||
| public static Map< | ||
| TestFrameworkInstrumentation, TestEventsHandler<TestSuiteDescriptor, TestDescriptor>> | ||
| HANDLERS = new HashMap<>(); |
There was a problem hiding this comment.
The lookup used to be thread-safe as this was a volatile field. Now that it is a map, we need to add make the field final and use a concurrent map:
public static final Map<TestFrameworkInstrumentation, TestEventsHandler<TestSuiteDescriptor, TestDescriptor>> HANDLERS = new ConcurrentHashMap<>();
There was a problem hiding this comment.
We could even go so far as to implement our own concurrent map that works efficiently with enum keys:
import java.util.AbstractMap;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicReferenceArray;
import javax.annotation.Nonnull;
public class ConcurrentEnumMap<K extends Enum<K>, V> implements Map<K, V> {
private final K[] enumConstants;
private final AtomicReferenceArray<V> values;
public ConcurrentEnumMap(Class<K> enumClass) {
this.enumConstants = enumClass.getEnumConstants();
this.values = new AtomicReferenceArray<>(enumConstants.length);
}
private int indexOf(Object key) {
if (key == null) {
throw new NullPointerException("Key must not be null");
}
if (key.getClass() != enumConstants[0].getDeclaringClass()) {
throw new IllegalArgumentException("Key has wrong enum type");
}
@SuppressWarnings("unchecked")
K castKey = (K) key;
return castKey.ordinal();
}
@Override
public V get(Object key) {
int idx = indexOf(key);
return values.get(idx);
}
@Override
public V put(K key, V value) {
int idx = indexOf(key);
return values.getAndSet(idx, value);
}
@Override
public void putAll(@Nonnull Map<? extends K, ? extends V> m) {
for (Map.Entry<? extends K,? extends V> e : m.entrySet()) {
put(e.getKey(), e.getValue());
}
}
@Override
public V remove(Object key) {
int idx = indexOf(key);
return values.getAndSet(idx, null);
}
@Override
public V putIfAbsent(@Nonnull K key, V value) {
int idx = indexOf(key);
for (;;) {
V existing = values.get(idx);
if (existing != null) {
return existing;
}
if (values.compareAndSet(idx, null, value)) {
return null;
}
}
}
@Override
public boolean remove(@Nonnull Object key, Object value) {
int idx = indexOf(key);
V cur = values.get(idx);
if (cur == null || !cur.equals(value)) {
return false;
}
return values.compareAndSet(idx, cur, null);
}
@Override
public boolean replace(@Nonnull K key, @Nonnull V oldValue, @Nonnull V newValue) {
int idx = indexOf(key);
return values.compareAndSet(idx, oldValue, newValue);
}
@Override
public V replace(@Nonnull K key, @Nonnull V value) {
int idx = indexOf(key);
for (;;) {
V cur = values.get(idx);
if (cur == null) {
return null;
}
if (values.compareAndSet(idx, cur, value)) {
return cur;
}
}
}
@Override
public int size() {
int count = 0;
for (int i = 0; i < values.length(); i++) {
if (values.get(i) != null) {
count++;
}
}
return count;
}
@Override
public boolean isEmpty() {
for (int i = 0; i < values.length(); i++) {
if (values.get(i) != null) {
return false;
}
}
return true;
}
@Override
public boolean containsKey(Object key) {
return get(key) != null;
}
@Override
public boolean containsValue(Object value) {
for (int i = 0; i < values.length(); i++) {
V v = values.get(i);
if (v != null && v.equals(value)) {
return true;
}
}
return false;
}
@Override
public void clear() {
for (int i = 0; i < values.length(); i++) {
values.set(i, null);
}
}
@Nonnull
@Override
public Set<K> keySet() {
Set<K> keys = EnumSet.noneOf(enumConstants[0].getDeclaringClass());
for (int i = 0; i < values.length(); i++) {
if (values.get(i) != null) {
keys.add(enumConstants[i]);
}
}
return Collections.unmodifiableSet(keys);
}
@Nonnull
@Override
public Collection<V> values() {
List<V> list = new ArrayList<>();
for (int i = 0; i < values.length(); i++) {
V value = values.get(i);
if (value != null) {
list.add(value);
}
}
return Collections.unmodifiableList(list);
}
@Nonnull
@Override
public Set<Map.Entry<K, V>> entrySet() {
Set<Map.Entry<K, V>> entrySet = new java.util.HashSet<>();
for (int i = 0; i < enumConstants.length; i++) {
V value = values.get(i);
if (value != null) {
K key = enumConstants[i];
entrySet.add(new AbstractMap.SimpleEntry<>(key, value));
}
}
return Collections.unmodifiableSet(entrySet);
}
}
| TEST_EVENTS_HANDLER = null; | ||
| for (Map.Entry< | ||
| TestFrameworkInstrumentation, TestEventsHandler<TestSuiteDescriptor, TestDescriptor>> | ||
| entry : HANDLERS.entrySet()) { |
There was a problem hiding this comment.
We can use HANDLERS.values() as we don't need the keys
| } | ||
| } | ||
|
|
||
| // used by instrumentation tests | ||
| public static synchronized void startForcefully() { | ||
| public static synchronized void startForcefully(TestFrameworkInstrumentation framework) { |
There was a problem hiding this comment.
I think with the new approach we can get rid of the startForcefully method.
The "forceful" method was only needed as the handler was started by default at class init time - basically the same problem that you encountered with JUnit 4.
Now that the handler is started on test engine discovery, it seems like we can completely drop the "start" call from the tests, leaving only the stop.
|
|
||
| public abstract class TestEventsHandlerHolder { | ||
|
|
||
| public static volatile TestEventsHandler<TestDescriptor, TestDescriptor> TEST_EVENTS_HANDLER; | ||
| // store one handler per framework running | ||
| public static Map<TestFrameworkInstrumentation, TestEventsHandler<TestDescriptor, TestDescriptor>> |
There was a problem hiding this comment.
Same comment as with JUnit 4 regarding thread safety: the field needs to be final and the map implementation - thread-safe.
| TEST_EVENTS_HANDLER.close(); | ||
| TEST_EVENTS_HANDLER = null; | ||
| for (Map.Entry<TestFrameworkInstrumentation, TestEventsHandler<TestDescriptor, TestDescriptor>> | ||
| entry : HANDLERS.entrySet()) { |
There was a problem hiding this comment.
Let's use HANDLERS.values()
| @@ -77,12 +78,12 @@ public static class ContextStoreAdvice { | |||
| value = "UC_USELESS_OBJECT", | |||
| justification = "executionRequest is the argument of the original method") | |||
| @Advice.OnMethodEnter | |||
| public static void setContextStores() { | |||
| public static void setContextStores(@Advice.This TestEngine testEngine) { | |||
| ContextStore<TestDescriptor, Object> contextStore = | |||
| InstrumentationContext.get(TestDescriptor.class, Object.class); | |||
| TestEventsHandlerHolder.setContextStores( | |||
There was a problem hiding this comment.
It looks like we can get rid of setContextStores method and the corresponding fields in the holder class - as they're only used when starting an event handler, we can pass them directly to start method as arguments
| @@ -20,14 +25,24 @@ public abstract class TestEventsHandlerHolder { | |||
| false)); | |||
| } | |||
|
|
|||
| public static void start() { | |||
| TEST_EVENTS_HANDLER = InstrumentationBridge.createTestEventsHandler("junit", null, null); | |||
| public static void start(TestFrameworkInstrumentation framework) { | |||
There was a problem hiding this comment.
The JUnit 5 implementation has a safety check to ensure we're not overwriting a handler that already exists. I cannot tell for sure if it is possible with JUnit 4 - either we need to ensure it is not, or add a check similar to the one in JUnit 5 (check if map entry exists, and only add a new one if it doesn't + have start and stop synchronised to ensure atomicity).
| @@ -112,7 +112,7 @@ public static class ExecutionAdvice { | |||
| @SuppressFBWarnings("NP_BOOLEAN_RETURN_NULL") | |||
| @Advice.OnMethodEnter(skipOn = Boolean.class) | |||
| public static Boolean execute(@Advice.This HierarchicalTestExecutorService.TestTask testTask) { | |||
| if (TestEventsHandlerHolder.TEST_EVENTS_HANDLER == null) { | |||
| if (TestEventsHandlerHolder.DEFAULT_HANDLER == null) { | |||
There was a problem hiding this comment.
Instead of having the DEFAULT_HANDLER we could try something like this:
TestTaskHandle taskHandle = new TestTaskHandle(testTask);
TestDescriptor testDescriptor = taskHandle.getTestDescriptor();
TestFrameworkInstrumentation testFramework;
UniqueId uniqueId = testDescriptor.getUniqueId();
String engineId = uniqueId.getEngineId().orElse(null);
if ("cucumber".equals(engineId)) {
testFramework = TestFrameworkInstrumentation.CUCUMBER;
} else if ("spock".equals(engineId)) {
testFramework = TestFrameworkInstrumentation.SPOCK;
} else {
testFramework = TestFrameworkInstrumentation.JUNIT5;
}
There was a problem hiding this comment.
The part that works with TestDescriptor can be moved to a utility method in JUnitPlatformUtils.
As a bonus, check the calls to datadog.trace.instrumentation.junit5.TestDataFactory#register and replace the "engine ID" string literals with constants defined in the utils class.
There was a problem hiding this comment.
Correction, uniqueId.getEngineId() returns the outermost engine ID, while we need the innermost one (it is possible to have multiple engine IDs in the same descriptor if, for example, you're using a suite engine to run a suite of Spock/Cucumber/JUnit 5 tests: in this case the outermost engine ID will be "junit-platform-suite", which is not what we want).
So instead of doing uniqueId.getEngineId() we'd need something similar to:
public static String getEngineId(TestDescriptor testDescriptor) {
UniqueId uniqueId = testDescriptor.getUniqueId();
List<UniqueId.Segment> segments = uniqueId.getSegments();
ListIterator<UniqueId.Segment> it = segments.listIterator(segments.size());
while (it.hasPrevious()) {
UniqueId.Segment segment = it.previous();
if ("engine".equals(segment.getType())) {
return segment.getValue();
}
}
return null;
}
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Comment on lines
94
to
102
| for (; ; ) { | ||
| V cur = values.get(idx); | ||
| if (cur == null) { | ||
| return null; | ||
| } | ||
| if (values.compareAndSet(idx, cur, value)) { | ||
| return cur; | ||
| } | ||
| } |
There was a problem hiding this comment.
⚪ Code Quality Violation
This for loop could be simplied to a while loop (...read more)
This rule identifies instances where simplifying for loops is possible which can make these loops more concise.
| TestFrameworkInstrumentation framework = JUnitPlatformUtils.engineToFramework(testEngine); | ||
| TestEventsHandler<TestDescriptor, TestDescriptor> handler = HANDLERS.get(framework); | ||
| if (handler == null) { | ||
| handler = InstrumentationBridge.createTestEventsHandler("junit", SUITE_STORE, TEST_STORE); | ||
| handler = InstrumentationBridge.createTestEventsHandler("junit", suiteStore, testStore); |
There was a problem hiding this comment.
Now that we have a handler per framework, I guess we can use something like framework.asString() for the component name instead of the hardcoded "junit" value
| public static synchronized void start(TestFrameworkInstrumentation framework) { | ||
| TestEventsHandler<TestSuiteDescriptor, TestDescriptor> handler = HANDLERS.get(framework); | ||
| if (handler == null) { | ||
| HANDLERS.put(framework, InstrumentationBridge.createTestEventsHandler("junit", null, null)); |
There was a problem hiding this comment.
Same here with the hardcoded "junit" component
nikita-tkachenko-datadog
approved these changes
Feb 27, 2025
|
The last missing piece is removing the |
|
Yes! Was working on it, but also fixed a small issue we had on the fixture generation logic. The code was updating the fixtures in |
daniel-mohedano
deleted the
daniel.mohedano/instrumentation-test-handling-refactor
branch
svc-squareup-copybara
pushed a commit
to cashapp/misk
that referenced
this pull request
Mar 6, 2025
| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [com.datadoghq:dd-trace-api](https://github.com/datadog/dd-trace-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.46.1` -> `1.47.0` | | [com.datadoghq:dd-trace-ot](https://github.com/datadog/dd-trace-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.46.1` -> `1.47.0` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.33` -> `2.30.34` | | [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.33` -> `2.30.34` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.33` -> `2.30.34` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.33` -> `2.30.34` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.33` -> `2.30.34` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.33` -> `2.30.34` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.33` -> `2.30.34` | --- ### Release Notes <details> <summary>datadog/dd-trace-java (com.datadoghq:dd-trace-api)</summary> ### [`v1.47.0`](https://github.com/DataDog/dd-trace-java/releases/tag/v1.47.0): 1.47.0 ##### Components ##### Application Security Management (IAST) - 🐛 Exclude com.stripe.net.HttpURLConnectionClient to solve IAST SSRF vulnerability false positives ([#​8483](DataDog/dd-trace-java#8483) - [@​jandro996](https://github.com/jandro996)) - 🐛 Add exclusion to solve IAST weak randomness vulnerability false positives ([#​8462](DataDog/dd-trace-java#8462) - [@​jandro996](https://github.com/jandro996)) - ✨ Fix weak randomness false positive in Kafka client ([#​8408](DataDog/dd-trace-java#8408) - [@​smola](https://github.com/smola)) - ✨ Fix location for SSRF with Kong Unirest ([#​8407](DataDog/dd-trace-java#8407) - [@​smola](https://github.com/smola)) - ✨ Exclude IBM Instana from IAST ([#​8406](DataDog/dd-trace-java#8406) - [@​smola](https://github.com/smola)) - 🐛 Fix org.json iast instrumentation test for latest dependency ([#​8347](DataDog/dd-trace-java#8347) - [@​jandro996](https://github.com/jandro996)) - ✨ Configuration to Disable APM Tracing ([#​8219](DataDog/dd-trace-java#8219) - [@​jandro996](https://github.com/jandro996)) - ✨ Address cookie vulnerability cardinality issues ([#​8210](DataDog/dd-trace-java#8210) - [@​jandro996](https://github.com/jandro996)) - ✨ Email HTML Injection detection in IAST ([#​8205](DataDog/dd-trace-java#8205) - [@​sezen-datadog](https://github.com/sezen-datadog)) ##### Application Security Management (WAF) - 🐛✨ Ensure usr.exists tag is not overridden when UsernameNotFoundException is thrown ([#​8376](DataDog/dd-trace-java#8376) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - 🐛✨ Ensure usr.exists tag is not overridden by auto instrumentation ([#​8374](DataDog/dd-trace-java#8374) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - ✨ Update appsec metrics with event_rules_version tag ([#​8354](DataDog/dd-trace-java#8354) - [@​sezen-datadog](https://github.com/sezen-datadog)) - ✨ Update metrics: appsec.waf.requests ([#​8353](DataDog/dd-trace-java#8353) - [@​Mariovido](https://github.com/Mariovido)) - ✨ Improve ASM support in vert.x 5.0 ([#​8285](DataDog/dd-trace-java#8285) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - ✨ Update metrics: appsec.waf.updates and appsec.waf.init ([#​8280](DataDog/dd-trace-java#8280) - [@​Mariovido](https://github.com/Mariovido)) - ✨ Configuration to Disable APM Tracing ([#​8219](DataDog/dd-trace-java#8219) - [@​jandro996](https://github.com/jandro996)) ##### Build & Tooling - 🐛 Do not generate Muzzle references for primitive arrays in method body ([#​8361](DataDog/dd-trace-java#8361) - [@​amarziali](https://github.com/amarziali)) - 📖 Improve dev env setup documentation for Windows ([#​8180](DataDog/dd-trace-java#8180) - [@​lucaspimentel](https://github.com/lucaspimentel)) ##### Continuous Integration Visibility - ✨ Add support for skip-EFD tagging ([#​8487](DataDog/dd-trace-java#8487) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - 🐛 Fix an NPE in Gradle Android instrumentation ([#​8484](DataDog/dd-trace-java#8484) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - ✨ Consider modified tests when applying fail-fast tests ordering ([#​8474](DataDog/dd-trace-java#8474) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - ✨ Implement tests reordering for TestNG ([#​8467](DataDog/dd-trace-java#8467) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - 🐛 Fix Gradle Launcher instrumentation to not interfere with Gradle Test Kit ([#​8465](DataDog/dd-trace-java#8465) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - 🧹 Use separate TestEventHandlers per framework in CI Vis instrumentations ([#​8451](DataDog/dd-trace-java#8451) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) - ✨ Remove warning log when JUnit 4 test method cannot be retrieved ([#​8445](DataDog/dd-trace-java#8445) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - 🐛 Fix Scalatest tracing for tests that are reported asynchronously ([#​8444](DataDog/dd-trace-java#8444) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - ✨ Implement attempt to fix tests ([#​8393](DataDog/dd-trace-java#8393) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) - ✨ Implement test disabling ([#​8377](DataDog/dd-trace-java#8377) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) - ✨ Update CODEOWNERS parser to not log errors on comments with leading whitespace ([#​8349](DataDog/dd-trace-java#8349) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - ✨ Request Test Management tests list ([#​8345](DataDog/dd-trace-java#8345) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) - ✨ Receive test management settings from CIVis settings request ([#​8331](DataDog/dd-trace-java#8331) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) - ✨ Implement quarantined tests tagging ([#​8326](DataDog/dd-trace-java#8326) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - ✨ Implement tests quarantining ([#​8320](DataDog/dd-trace-java#8320) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - ✨ Add tag to specify if the user is setting DD_SERVICE ([#​8318](DataDog/dd-trace-java#8318) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) ##### Crash tracking - ✨ Only fork jps when required ([#​8419](DataDog/dd-trace-java#8419) - [@​mcculls](https://github.com/mcculls)) - 🐛 Use Java home of the crashed process to launch crash uploader ([#​8348](DataDog/dd-trace-java#8348) - [@​jbachorik](https://github.com/jbachorik)) ##### Data Streams Monitoring - 🐛 Fix error happening when sqs message attributes are readonly ([#​8473](DataDog/dd-trace-java#8473) - [@​vandonr](https://github.com/vandonr)) - 🐛 Fix bug on proto schema extraction ([#​8403](DataDog/dd-trace-java#8403) - [@​vandonr](https://github.com/vandonr)) - 🐛 Fix service name overrides in consumers ([#​8387](DataDog/dd-trace-java#8387) - [@​piochelepiotr](https://github.com/piochelepiotr)) ##### Database Monitoring - ✨ Add DBMTracePreparedStatements to tracer configuration log ([#​8508](DataDog/dd-trace-java#8508) - [@​cecile75](https://github.com/cecile75)) ##### Dynamic Instrumentation - ✨ Look in another location for grpc service methods ([#​8468](DataDog/dd-trace-java#8468) - [@​evanchooly](https://github.com/evanchooly)) - 🐛 Fix Exception Replay with Lambda proxy classes ([#​8452](DataDog/dd-trace-java#8452) - [@​jpbempel](https://github.com/jpbempel)) - ✨ Add code origin support for spring-webmvc ([#​8416](DataDog/dd-trace-java#8416) - [@​evanchooly](https://github.com/evanchooly)) - ✨ Add support for scanning jar from loaded class ([#​8370](DataDog/dd-trace-java#8370) - [@​jpbempel](https://github.com/jpbempel)) - 🐛 Disable capture of entry values ([#​8369](DataDog/dd-trace-java#8369) - [@​jpbempel](https://github.com/jpbempel)) - 🐛 Fix CodeOrigin for `@Trace` annotation ([#​8344](DataDog/dd-trace-java#8344) - [@​jpbempel](https://github.com/jpbempel)) - 🐛 Fix equals/hashCode for CodeOrigin probe ([#​8319](DataDog/dd-trace-java#8319) - [@​jpbempel](https://github.com/jpbempel)) - ✨ Add code origin support to kafka message listeners ([#​8301](DataDog/dd-trace-java#8301) - [@​evanchooly](https://github.com/evanchooly)) ##### Metrics - ✨ Create metric: appsec.waf.error ([#​8381](DataDog/dd-trace-java#8381) - [@​sezen-datadog](https://github.com/sezen-datadog)) - ✨ Create metric: appsec.rasp.error ([#​8364](DataDog/dd-trace-java#8364) - [@​sezen-datadog](https://github.com/sezen-datadog)) ##### Profiling - ✨ Bump ddprof library to 1.22.0 ([#​8463](DataDog/dd-trace-java#8463) - [@​jbachorik](https://github.com/jbachorik)) - IBM J9 8u361 corresponds to OpenJDK 8u362 by [@​jbachorik](https://github.com/jbachorik) in DataDog/java-profiler#187 - Fix compatibility with musl libc 1.2.4 by [@​jbachorik](https://github.com/jbachorik) in DataDog/java-profiler#189 - Modify version extraction by [@​jbachorik](https://github.com/jbachorik) in DataDog/java-profiler#179 - Do not write null values to jvminfo event by [@​jbachorik](https://github.com/jbachorik) in DataDog/java-profiler#184 - Productize VMStructs-based stack walker by [@​jbachorik](https://github.com/jbachorik) in DataDog/java-profiler#177 - A few minor downport issues by [@​jbachorik](https://github.com/jbachorik) in DataDog/java-profiler#180 - Enable ASGCT by default on fairly safe J9 JDK versions by [@​jbachorik](https://github.com/jbachorik) in DataDog/java-profiler#181 - 🐛 Exclude OrderedThreadPoolExecutor from queue-time measurements ([#​8456](DataDog/dd-trace-java#8456) - [@​jbachorik](https://github.com/jbachorik)) - ✨ Record JVM info on JVMs without JFR ([#​8431](DataDog/dd-trace-java#8431) - [@​jbachorik](https://github.com/jbachorik)) - 🐛 Actually use CleanupTask in TempLocationManager ([#​8420](DataDog/dd-trace-java#8420) - [@​mcculls](https://github.com/mcculls)) - ✨ Only fork jps when required ([#​8419](DataDog/dd-trace-java#8419) - [@​mcculls](https://github.com/mcculls)) - 🐛 Adjust JFR checks for J9 ([#​8405](DataDog/dd-trace-java#8405) - [@​jbachorik](https://github.com/jbachorik)) - 🧹 Disable smap RSS parsing by default ([#​8342](DataDog/dd-trace-java#8342) - [@​MattAlp](https://github.com/MattAlp)) ##### Telemetry - 🐛 Add support for JBoss jar:file format to DependencyResolver ([#​8428](DataDog/dd-trace-java#8428) - [@​jandro996](https://github.com/jandro996)) - ✨ Update metrics: appsec.waf.requests ([#​8353](DataDog/dd-trace-java#8353) - [@​Mariovido](https://github.com/Mariovido)) ##### Trace context propagation - ✨ Introduce tracing propagator ([#​8313](DataDog/dd-trace-java#8313) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) ##### Tracer core - 🐛 Fix Stable Config telemetry source names ([#​8460](DataDog/dd-trace-java#8460) - [@​BaptisteFoy](https://github.com/BaptisteFoy)) - ✨ Probe trace endpoints with a valid payload of empty arrays ([#​8414](DataDog/dd-trace-java#8414) - [@​mcculls](https://github.com/mcculls)) - ✨ Add 1 minute fail-safe to JUL/JMX class-loading callback ([#​8399](DataDog/dd-trace-java#8399) - [@​mcculls](https://github.com/mcculls)) - ✨ Migrate DSM injection calls to context-first APIs ([#​8383](DataDog/dd-trace-java#8383) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) - 🧹 Move continuation capture methods from scope to tracer ([#​8371](DataDog/dd-trace-java#8371) - [@​mcculls](https://github.com/mcculls)) - ✨ Migrate context extraction calls to context-first APIs ([#​8368](DataDog/dd-trace-java#8368) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) - 🧹 Migrate context injection calls to context-first APIs ([#​8358](DataDog/dd-trace-java#8358) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) - 💡 Support reading configurations from files ([#​8338](DataDog/dd-trace-java#8338) - [@​mtoffl01](https://github.com/mtoffl01)) - 💡 Implementation of BaggagePropagator and BaggageContext ([#​8330](DataDog/dd-trace-java#8330) - [@​mhlidd](https://github.com/mhlidd)) - 🧹 Combine continuation implementations into one which supports multiple activations ([#​8324](DataDog/dd-trace-java#8324) - [@​mcculls](https://github.com/mcculls)) - ✨ Introduce tracing propagator ([#​8313](DataDog/dd-trace-java#8313) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) - ✨ Remove old context propagation API ([#​8271](DataDog/dd-trace-java#8271) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) ##### Instrumentations ##### AWS Lambda instrumentation - 🐛 Send error message and stack to Lambda extension ([#​8417](DataDog/dd-trace-java#8417) - [@​nhulston](https://github.com/nhulston)) ##### AWS SDK instrumentation - 🐛 Fix error happening when sqs message attributes are readonly ([#​8473](DataDog/dd-trace-java#8473) - [@​vandonr](https://github.com/vandonr)) - 💡 Inject trace context into AWS Step Functions input ([#​7585](DataDog/dd-trace-java#7585) - [@​DylanLovesCoffee](https://github.com/DylanLovesCoffee)) ##### Core Java language instrumentation - ✨ Look in another location for grpc service methods ([#​8468](DataDog/dd-trace-java#8468) - [@​evanchooly](https://github.com/evanchooly)) - ✨ Add code origin support for spring-webmvc ([#​8416](DataDog/dd-trace-java#8416) - [@​evanchooly](https://github.com/evanchooly)) - 💡 Implementation of BaggagePropagator and BaggageContext ([#​8330](DataDog/dd-trace-java#8330) - [@​mhlidd](https://github.com/mhlidd)) - ✨ Add code origin support to kafka message listeners ([#​8301](DataDog/dd-trace-java#8301) - [@​evanchooly](https://github.com/evanchooly)) ##### gRPC instrumentation - ✨ Look in another location for grpc service methods ([#​8468](DataDog/dd-trace-java#8468) - [@​evanchooly](https://github.com/evanchooly)) ##### Kafka instrumentation - ✨ Add messaging.destination.name tag to kafka integrations ([#​8366](DataDog/dd-trace-java#8366) - [@​rarguelloF](https://github.com/rarguelloF)) ##### Protocol Buffer instrumentation - 🐛 Fix bug on proto schema extraction ([#​8403](DataDog/dd-trace-java#8403) - [@​vandonr](https://github.com/vandonr)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Never, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 108a0f86aa59ab4c938cbac0688dd4c19cb301fa
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Does This Do
Motivation
Refactored in preparation for libraries capabilities tagging. Different frameworks might have different capabilities, which causes problems if the TestEventHandler is reused between them.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: SDTEST-1616