Add capabilities tagging #8499
Merged
Add capabilities tagging #8499
+1,619
−109
Conversation
This file contains 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
# Conflicts: # dd-java-agent/agent-ci-visibility/src/testFixtures/groovy/datadog/trace/civisibility/CiVisibilityInstrumentationTest.groovy
daniel-mohedano
added
type: enhancement
comp: ci visibility
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| } | ||
|
|
||
| private static class ListItem extends ArrayList<Item> implements Item { | ||
| private ListItem() {} |
There was a problem hiding this comment.
⚪ Code Quality Violation
Consider adding super() or this() to your constructor (...read more)
In Java, it is suggested to call super() in an extended class. This rule will report a violation if both a call to super() and an overloaded constructor are absent.
| } | ||
| } | ||
|
|
||
| buffer.append("]"); |
There was a problem hiding this comment.
🟠 Code Quality Violation
Suggested change
| buffer.append("]"); | |
| buffer.append(']'); |
Do not append a string literal that contains only a single char (...read more)
Do not append a string with single character to a StringBuffer. Instead, append a character type. This is better practice and results in better performance. While this is negligible if the operation occurs once, it has performance impact if done at scale.
Learn More
|
|
||
| private String toListString() { | ||
| StringBuilder buffer = new StringBuilder(); | ||
| buffer.append("["); |
There was a problem hiding this comment.
🟠 Code Quality Violation
Suggested change
| buffer.append("["); | |
| buffer.append('['); |
Do not append a string literal that contains only a single char (...read more)
Do not append a string with single character to a StringBuffer. Instead, append a character type. This is better practice and results in better performance. While this is negligible if the operation occurs once, it has performance impact if done at scale.
Learn More
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 56 metrics, 7 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.046 s) : 0, 1046382
Total [baseline] (8.681 s) : 0, 8680563
Agent [candidate] (1.044 s) : 0, 1044432
Total [candidate] (8.656 s) : 0, 8656047
section iast
Agent [baseline] (1.17 s) : 0, 1169774
Total [baseline] (9.226 s) : 0, 9225983
Agent [candidate] (1.172 s) : 0, 1172017
Total [candidate] (9.258 s) : 0, 9258079
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.176 s) : 0, 1176397
Total [baseline] (9.212 s) : 0, 9211735
Agent [candidate] (1.174 s) : 0, 1174376
Total [candidate] (9.22 s) : 0, 9219601
section iast_TELEMETRY_OFF
Agent [baseline] (1.173 s) : 0, 1173232
Total [baseline] (9.247 s) : 0, 9246979
Agent [candidate] (1.178 s) : 0, 1177518
Total [candidate] (9.251 s) : 0, 9250578
gantt
title insecure-bank - break down per module: candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (720.863 ms) : 0, 720863
BytebuddyAgent [candidate] (721.991 ms) : 0, 721991
GlobalTracer [baseline] (240.112 ms) : 0, 240112
GlobalTracer [candidate] (241.049 ms) : 0, 241049
AppSec [baseline] (55.775 ms) : 0, 55775
AppSec [candidate] (55.647 ms) : 0, 55647
Remote Config [baseline] (703.182 µs) : 0, 703
Remote Config [candidate] (695.625 µs) : 0, 696
Telemetry [baseline] (13.83 ms) : 0, 13830
Telemetry [candidate] (10.038 ms) : 0, 10038
section iast
BytebuddyAgent [baseline] (836.685 ms) : 0, 836685
BytebuddyAgent [candidate] (837.182 ms) : 0, 837182
GlobalTracer [baseline] (229.998 ms) : 0, 229998
GlobalTracer [candidate] (230.567 ms) : 0, 230567
IAST [baseline] (22.707 ms) : 0, 22707
IAST [candidate] (22.949 ms) : 0, 22949
AppSec [baseline] (56.272 ms) : 0, 56272
AppSec [candidate] (56.92 ms) : 0, 56920
Remote Config [baseline] (618.203 µs) : 0, 618
Remote Config [candidate] (619.859 µs) : 0, 620
Telemetry [baseline] (8.601 ms) : 0, 8601
Telemetry [candidate] (8.798 ms) : 0, 8798
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (841.728 ms) : 0, 841728
BytebuddyAgent [candidate] (839.712 ms) : 0, 839712
GlobalTracer [baseline] (230.398 ms) : 0, 230398
GlobalTracer [candidate] (230.78 ms) : 0, 230780
IAST [baseline] (23.031 ms) : 0, 23031
IAST [candidate] (22.857 ms) : 0, 22857
AppSec [baseline] (56.86 ms) : 0, 56860
AppSec [candidate] (56.664 ms) : 0, 56664
Remote Config [baseline] (613.16 µs) : 0, 613
Remote Config [candidate] (618.34 µs) : 0, 618
Telemetry [baseline] (8.679 ms) : 0, 8679
Telemetry [candidate] (8.704 ms) : 0, 8704
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (838.742 ms) : 0, 838742
BytebuddyAgent [candidate] (842.249 ms) : 0, 842249
GlobalTracer [baseline] (231.023 ms) : 0, 231023
GlobalTracer [candidate] (231.611 ms) : 0, 231611
IAST [baseline] (22.357 ms) : 0, 22357
IAST [candidate] (25.642 ms) : 0, 25642
AppSec [baseline] (56.79 ms) : 0, 56790
AppSec [candidate] (53.596 ms) : 0, 53596
Remote Config [baseline] (621.749 µs) : 0, 622
Remote Config [candidate] (624.002 µs) : 0, 624
Telemetry [baseline] (8.653 ms) : 0, 8653
Telemetry [candidate] (8.691 ms) : 0, 8691
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.039 s) : 0, 1038900
Total [baseline] (10.442 s) : 0, 10442111
Agent [candidate] (1.05 s) : 0, 1049659
Total [candidate] (10.521 s) : 0, 10520899
section appsec
Agent [baseline] (1.183 s) : 0, 1183254
Total [baseline] (10.772 s) : 0, 10772018
Agent [candidate] (1.188 s) : 0, 1188470
Total [candidate] (10.831 s) : 0, 10831380
section iast
Agent [baseline] (1.172 s) : 0, 1171619
Total [baseline] (10.971 s) : 0, 10971092
Agent [candidate] (1.17 s) : 0, 1170023
Total [candidate] (10.954 s) : 0, 10954245
section profiling
Agent [baseline] (1.27 s) : 0, 1269773
Total [baseline] (10.891 s) : 0, 10890971
Agent [candidate] (1.26 s) : 0, 1259846
Total [candidate] (10.843 s) : 0, 10843251
gantt
title petclinic - break down per module: candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (715.763 ms) : 0, 715763
BytebuddyAgent [candidate] (722.65 ms) : 0, 722650
GlobalTracer [baseline] (239.361 ms) : 0, 239361
GlobalTracer [candidate] (241.252 ms) : 0, 241252
AppSec [baseline] (55.227 ms) : 0, 55227
AppSec [candidate] (55.41 ms) : 0, 55410
Remote Config [baseline] (686.808 µs) : 0, 687
Remote Config [candidate] (692.188 µs) : 0, 692
Telemetry [baseline] (12.891 ms) : 0, 12891
Telemetry [candidate] (14.58 ms) : 0, 14580
section appsec
BytebuddyAgent [baseline] (735.088 ms) : 0, 735088
BytebuddyAgent [candidate] (738.119 ms) : 0, 738119
GlobalTracer [baseline] (236.196 ms) : 0, 236196
GlobalTracer [candidate] (237.599 ms) : 0, 237599
AppSec [baseline] (177.333 ms) : 0, 177333
AppSec [candidate] (177.465 ms) : 0, 177465
Remote Config [baseline] (663.297 µs) : 0, 663
Remote Config [candidate] (682.673 µs) : 0, 683
Telemetry [baseline] (8.289 ms) : 0, 8289
Telemetry [candidate] (8.727 ms) : 0, 8727
IAST [baseline] (21.555 ms) : 0, 21555
IAST [candidate] (21.84 ms) : 0, 21840
section iast
BytebuddyAgent [baseline] (837.523 ms) : 0, 837523
BytebuddyAgent [candidate] (836.295 ms) : 0, 836295
GlobalTracer [baseline] (230.608 ms) : 0, 230608
GlobalTracer [candidate] (229.998 ms) : 0, 229998
AppSec [baseline] (53.969 ms) : 0, 53969
AppSec [candidate] (56.852 ms) : 0, 56852
Remote Config [baseline] (617.122 µs) : 0, 617
Remote Config [candidate] (615.846 µs) : 0, 616
Telemetry [baseline] (8.657 ms) : 0, 8657
Telemetry [candidate] (8.645 ms) : 0, 8645
IAST [baseline] (25.23 ms) : 0, 25230
IAST [candidate] (22.729 ms) : 0, 22729
section profiling
BytebuddyAgent [baseline] (715.024 ms) : 0, 715024
BytebuddyAgent [candidate] (709.366 ms) : 0, 709366
GlobalTracer [baseline] (351.625 ms) : 0, 351625
GlobalTracer [candidate] (350.181 ms) : 0, 350181
AppSec [baseline] (55.492 ms) : 0, 55492
AppSec [candidate] (54.079 ms) : 0, 54079
Remote Config [baseline] (693.518 µs) : 0, 694
Remote Config [candidate] (668.63 µs) : 0, 669
Telemetry [baseline] (9.025 ms) : 0, 9025
Telemetry [candidate] (8.995 ms) : 0, 8995
ProfilingAgent [baseline] (97.238 ms) : 0, 97238
ProfilingAgent [candidate] (96.193 ms) : 0, 96193
Profiling [baseline] (97.265 ms) : 0, 97265
Profiling [candidate] (96.218 ms) : 0, 96218
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 1 performance regressions! Performance is the same for 13 metrics, 16 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section baseline
no_agent (383.64 µs) : 364, 403
. : milestone, 384,
iast (517.036 µs) : 495, 539
. : milestone, 517,
iast_FULL (738.833 µs) : 717, 761
. : milestone, 739,
iast_GLOBAL (566.186 µs) : 544, 588
. : milestone, 566,
iast_HARDCODED_SECRET_DISABLED (521.829 µs) : 499, 544
. : milestone, 522,
iast_INACTIVE (468.442 µs) : 447, 490
. : milestone, 468,
iast_TELEMETRY_OFF (504.232 µs) : 482, 526
. : milestone, 504,
tracing (461.176 µs) : 441, 482
. : milestone, 461,
section candidate
no_agent (384.707 µs) : 365, 404
. : milestone, 385,
iast (518.291 µs) : 496, 540
. : milestone, 518,
iast_FULL (737.098 µs) : 715, 759
. : milestone, 737,
iast_GLOBAL (572.27 µs) : 551, 594
. : milestone, 572,
iast_HARDCODED_SECRET_DISABLED (518.948 µs) : 497, 541
. : milestone, 519,
iast_INACTIVE (469.011 µs) : 447, 491
. : milestone, 469,
iast_TELEMETRY_OFF (502.991 µs) : 480, 526
. : milestone, 503,
tracing (459.748 µs) : 439, 481
. : milestone, 460,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section baseline
no_agent (1.355 ms) : 1335, 1376
. : milestone, 1355,
appsec (1.734 ms) : 1710, 1757
. : milestone, 1734,
appsec_no_iast (1.751 ms) : 1727, 1776
. : milestone, 1751,
code_origins (1.687 ms) : 1653, 1721
. : milestone, 1687,
iast (1.517 ms) : 1492, 1542
. : milestone, 1517,
profiling (1.497 ms) : 1473, 1520
. : milestone, 1497,
tracing (1.49 ms) : 1465, 1516
. : milestone, 1490,
section candidate
no_agent (1.381 ms) : 1361, 1401
. : milestone, 1381,
appsec (1.742 ms) : 1717, 1766
. : milestone, 1742,
appsec_no_iast (1.743 ms) : 1718, 1768
. : milestone, 1743,
code_origins (1.687 ms) : 1653, 1721
. : milestone, 1687,
iast (1.52 ms) : 1496, 1544
. : milestone, 1520,
profiling (1.562 ms) : 1538, 1587
. : milestone, 1562,
tracing (1.498 ms) : 1472, 1523
. : milestone, 1498,
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 biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section baseline
no_agent (15.458 s) : 15458000, 15458000
. : milestone, 15458000,
appsec (14.957 s) : 14957000, 14957000
. : milestone, 14957000,
iast (18.177 s) : 18177000, 18177000
. : milestone, 18177000,
iast_GLOBAL (17.884 s) : 17884000, 17884000
. : milestone, 17884000,
profiling (14.913 s) : 14913000, 14913000
. : milestone, 14913000,
tracing (15.118 s) : 15118000, 15118000
. : milestone, 15118000,
section candidate
no_agent (15.396 s) : 15396000, 15396000
. : milestone, 15396000,
appsec (14.714 s) : 14714000, 14714000
. : milestone, 14714000,
iast (18.679 s) : 18679000, 18679000
. : milestone, 18679000,
iast_GLOBAL (18.2 s) : 18200000, 18200000
. : milestone, 18200000,
profiling (15.244 s) : 15244000, 15244000
. : milestone, 15244000,
tracing (14.849 s) : 14849000, 14849000
. : milestone, 14849000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.48.0-SNAPSHOT~02fa357a1d, baseline=1.48.0-SNAPSHOT~4eaa53be19
dateFormat X
axisFormat %s
section baseline
no_agent (1.472 ms) : 1461, 1484
. : milestone, 1472,
appsec (2.344 ms) : 2300, 2387
. : milestone, 2344,
iast (2.11 ms) : 2055, 2165
. : milestone, 2110,
iast_GLOBAL (2.158 ms) : 2103, 2213
. : milestone, 2158,
profiling (1.987 ms) : 1943, 2031
. : milestone, 1987,
tracing (1.946 ms) : 1904, 1988
. : milestone, 1946,
section candidate
no_agent (1.471 ms) : 1459, 1482
. : milestone, 1471,
appsec (2.347 ms) : 2304, 2391
. : milestone, 2347,
iast (2.11 ms) : 2055, 2165
. : milestone, 2110,
iast_GLOBAL (2.157 ms) : 2101, 2212
. : milestone, 2157,
profiling (1.991 ms) : 1946, 2035
. : milestone, 1991,
tracing (1.945 ms) : 1903, 1986
. : milestone, 1945,
|
| @@ -167,9 +169,11 @@ private TestEventsHandlerFactory( | |||
| public <SuiteKey, TestKey> TestEventsHandler<SuiteKey, TestKey> create( | |||
| String component, | |||
| @Nullable ContextStore<SuiteKey, DDTestSuite> suiteStore, | |||
| @Nullable ContextStore<TestKey, DDTest> testStore) { | |||
| @Nullable ContextStore<TestKey, DDTest> testStore, | |||
| Collection<LibraryCapability> availableCapabilities) { | |||
There was a problem hiding this comment.
I'd argue that a capability is available by definition :)
Suggested change
| Collection<LibraryCapability> availableCapabilities) { | |
| Collection<LibraryCapability> capabilities) { |
| @@ -144,6 +148,11 @@ public TestImpl( | |||
| span.setTag(Tags.ITR_CORRELATION_ID, itrCorrelationId); | |||
| } | |||
|
|
|||
| for (Map.Entry<LibraryCapability, Boolean> entry : libraryCapabilities.entrySet()) { | |||
| String capabilityTag = LibraryCapabilityUtils.CAPABILITY_TAG_MAP.get(entry.getKey()); | |||
There was a problem hiding this comment.
Rather than doing a map lookup, can't we store the tag as a field in LibraryCapability instances and then do something like entry.getKey().asTag()?
There was a problem hiding this comment.
I separated the LibraryCapability from the tags for encapsulation in the internal api, but I agree that it ended up making everything quite complicated to follow, so probably best to do as you say to also remove LibraryCapabilityUtils
|
|
||
| private void propagateTestTags(AgentSpan testSpan) { | ||
| SpanUtils.propagateCiVisibilityTags(span, testSpan); | ||
| SpanUtils.propagateLibraryCapabilities(span, testSpan); |
There was a problem hiding this comment.
Can't we call propagateLibraryCapabilities from propagateCiVisibilityTags? That'd allow us to centralise the logic and have less places to update
There was a problem hiding this comment.
On the other hand, if capabilities are already available in session, module, and suite, why do we propagate them from a child span rather than setting them directly in constructor along with the other tags?
There was a problem hiding this comment.
As discussed offline, let's remove the propagation altogether, as the spec only mandates that the capabilities be present in the test events
| @@ -144,6 +148,11 @@ public TestImpl( | |||
| span.setTag(Tags.ITR_CORRELATION_ID, itrCorrelationId); | |||
| } | |||
|
|
|||
| for (Map.Entry<LibraryCapability, Boolean> entry : libraryCapabilities.entrySet()) { | |||
There was a problem hiding this comment.
If I understood correctly, we might end up with a null here (coming from the ManualApiTestModule) and fail with a NullPointerException.
I'd say we should mark the argument as @Nonnull to let the compiler warn us about these things
| @@ -220,6 +223,30 @@ private LinesResolver.Lines getLines(Method testMethod) { | |||
| } | |||
| } | |||
|
|
|||
| public Map<LibraryCapability, Boolean> getCapabilitiesStatus() { | |||
There was a problem hiding this comment.
I'd make this method accept a collection of capabilities and would do the filtering inside it (that way we can get rid of LibraryCapabilitiesUtils which seems kind of clumsy.
There was a problem hiding this comment.
Moreover, we could add a Predicate<ExecutionSettings> field to LibraryCapability, then the method would look like:
public Map<LibraryCapability, Boolean> getCapabilitiesStatus(Collection<LibraryCapability> capabilities) {
Map<LibraryCapability, Boolean> status = new EnumMap<>(LibraryCapability.class);
for (LibraryCapability c : capabilities) {
status.put(c, c.status(executionSettings));
}
return status;
}
It may look a bit involved, but the advantage is that whenever you add a new LibraryCapability you will be forced to provide the predicate, and this method will stay untouched. This is more in line with the open-closed OOP principle
| @@ -31,7 +31,10 @@ public static synchronized void start(TestFrameworkInstrumentation framework) { | |||
| HANDLERS.put( | |||
There was a problem hiding this comment.
I'd say let's make the start method accept the list of capabilities. And then locate the hardcoded lists in the utility classes for each corresponding framework (JUnit, MUnit, Cucumber). Having MUnit and Cucumber capabilities hard-coded in JUnit utils feels off
There was a problem hiding this comment.
Same goes for JUnit5
| DDTags.LIBRARY_CAPABILITIES_EFD, | ||
| DDTags.LIBRARY_CAPABILITIES_IMPACTED_TESTS, | ||
| DDTags.LIBRARY_CAPABILITIES_QUARANTINE, | ||
| DDTags.LIBRARY_CAPABILITIES_ATTEMPT_TO_FIX |
There was a problem hiding this comment.
Once we have the list of capabilities hardcoded in MUnit utils we can rewrite the tests to avoid hardcoding the tags:
public abstract class MUnitUtils {
public static final Collection<LibraryCapability> CAPABILITIES = Arrays.asList(LibraryCapability.A, LibraryCapability.B);
}
public void test() {
arePresent(Arrays.stream(LibraryCapability.values()).map(LibraryCapability::asTag).collect(Collectors.toList()));
areNotPresent(Arrays.stream(LibraryCapability.values()).filter(capability -> !MUnitUtils.CAPABILITIES.contains(capability)).map(LibraryCapability::asTag).collect(Collectors.toList()));
}
There was a problem hiding this comment.
Also, we should be asserting the tags on test events, not sessions
...isibility/src/main/java/datadog/trace/civisibility/domain/manualapi/ManualApiTestModule.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Nikita Tkachenko <121111529+nikita-tkachenko-datadog@users.noreply.github.com>
nikita-tkachenko-datadog
approved these changes
Mar 6, 2025
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
Implements capabilities tagging for all CiVisibility/Test Optimization advanced features. The tagging follows the logic:
Motivation
This tags allow us to give some feedback to the customers about which advanced features they can enable and which ones they have enabled.
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