Skip to content
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

Add capabilities tagging #8499

Merged
merged 44 commits into from
Mar 7, 2025
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
8e9bb96
introduce seperate test events handlers in junit5 instrumentations
daniel-mohedano Feb 25, 2025
7ab85a0
clear junit5 handler map on close
daniel-mohedano Feb 25, 2025
b8c748f
introduce separate test events handlers in junit4 instrumentations
daniel-mohedano Feb 25, 2025
45735e6
junit4 instrumentation fixes
daniel-mohedano Feb 26, 2025
ee63fa7
fix muzzle checks for junit5 instrumentation
daniel-mohedano Feb 26, 2025
8ec555d
fix double close of default handler in junit5 instrumentation
daniel-mohedano Feb 26, 2025
04a8084
pr fixes
daniel-mohedano Feb 26, 2025
408a7d3
fix code quality violation
daniel-mohedano Feb 27, 2025
7d28e5a
use framework as component in junit4 and junit5 instrumentations
daniel-mohedano Feb 27, 2025
cfd4fda
update test fixtures with new component string
daniel-mohedano Feb 27, 2025
7877125
update smoke test fixtures with new component string
daniel-mohedano Feb 27, 2025
5a796b9
update selenium test fixtures and fix fixture generation
daniel-mohedano Feb 27, 2025
9a320be
fix codenarc issue
daniel-mohedano Feb 27, 2025
02c6bcc
Merge branch 'master' into daniel.mohedano/instrumentation-test-handl…
daniel-mohedano Feb 28, 2025
568f709
implements capabilities tagging logic
daniel-mohedano Feb 28, 2025
2b25ea8
update weaver instrumentation
daniel-mohedano Feb 28, 2025
ef2664d
update karate instrumentation
daniel-mohedano Feb 28, 2025
3538287
unify karate version in KarateUtils
daniel-mohedano Feb 28, 2025
a52cc21
update testng instrumentation
daniel-mohedano Feb 28, 2025
bc0d0b3
add testng ordering to capabilities
daniel-mohedano Feb 28, 2025
c9a0114
backport ComparableVersion from maven artifact to avoid packaging it
daniel-mohedano Feb 28, 2025
8129852
update junit5 instrumentations
daniel-mohedano Feb 28, 2025
103a089
update junit4 instrumentations
daniel-mohedano Feb 28, 2025
6685a5b
extend headless test session to ensure all capabilities are propagated
daniel-mohedano Feb 28, 2025
79df146
update scalatest instrumentation
daniel-mohedano Feb 28, 2025
063b82a
tag ignoring in span assertion and update smoke test fixtures
daniel-mohedano Mar 4, 2025
c0cf509
jacoco test coverage excludes
daniel-mohedano Mar 4, 2025
e12b7c8
Merge branch 'master' into daniel.mohedano/library-capabilities-tagging
daniel-mohedano Mar 4, 2025
6792874
fix code quality violations
daniel-mohedano Mar 4, 2025
2bee4c2
remove leftover maven dependency for karate
daniel-mohedano Mar 4, 2025
bc8ab0c
bugs and spotless fix
daniel-mohedano Mar 4, 2025
bf6d237
ignore tags with JsonPaths instead of custom implementation
daniel-mohedano Mar 4, 2025
b073736
clean up arePresent and areNotPresent in TagsAssert
daniel-mohedano Mar 4, 2025
0de9f87
add check for null framework versions
daniel-mohedano Mar 4, 2025
877fc63
remove capabilites propagation
daniel-mohedano Mar 4, 2025
8f6e0d7
remove unused test name for execution strategy
daniel-mohedano Mar 4, 2025
2e9d18c
PR changes
daniel-mohedano Mar 5, 2025
4238c3b
fix execution settings tests with sys config injection
daniel-mohedano Mar 5, 2025
0dcea22
fix TestImplTest
daniel-mohedano Mar 5, 2025
c31a7d7
Merge branch 'master' into daniel.mohedano/library-capabilities-tagging
daniel-mohedano Mar 5, 2025
f7aa755
fix master merge
daniel-mohedano Mar 5, 2025
28c2310
remove library capability utils
daniel-mohedano Mar 5, 2025
bd09eef
spotless !
daniel-mohedano Mar 5, 2025
02fa357
change empty map call
daniel-mohedano Mar 6, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import datadog.trace.api.civisibility.DDTest;
import datadog.trace.api.civisibility.DDTestSuite;
import datadog.trace.api.civisibility.InstrumentationBridge;
import datadog.trace.api.civisibility.config.LibraryCapability;
import datadog.trace.api.civisibility.coverage.CoveragePerTestBridge;
import datadog.trace.api.civisibility.events.BuildEventsHandler;
import datadog.trace.api.civisibility.events.TestEventsHandler;
Expand Down Expand Up @@ -39,6 +40,7 @@
import java.lang.instrument.Instrumentation;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Collection;
import java.util.function.Predicate;
import javax.annotation.Nullable;
import org.slf4j.Logger;
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that a capability is available by definition :)

Suggested change
Collection<LibraryCapability> availableCapabilities) {
Collection<LibraryCapability> capabilities) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very true 👍

TestFrameworkSession testSession =
sessionFactory.startSession(repoServices.moduleName, component, null);
sessionFactory.startSession(
repoServices.moduleName, component, null, availableCapabilities);
TestFrameworkModule testModule = testSession.testModuleStart(repoServices.moduleName, null);
return new TestEventsHandlerImpl<>(
services.metricCollector,
Expand Down Expand Up @@ -231,7 +235,10 @@ private static TestFrameworkSession.Factory childTestFrameworkSessionFactory(
CiVisibilityRepoServices repoServices,
CiVisibilityCoverageServices.Child coverageServices,
ExecutionSettings executionSettings) {
return (String projectName, String component, Long startTime) -> {
return (String projectName,
String component,
Long startTime,
Collection<LibraryCapability> availableCapabilities) -> {
String sessionName = services.config.getCiVisibilitySessionName();
String testCommand = services.config.getCiVisibilityTestCommand();
TestDecorator testDecorator =
Expand All @@ -255,7 +262,8 @@ private static TestFrameworkSession.Factory childTestFrameworkSessionFactory(
coverageServices.coverageStoreFactory,
coverageServices.coverageReporter,
services.signalClientFactory,
executionStrategy);
executionStrategy,
availableCapabilities);
};
}

Expand All @@ -264,7 +272,10 @@ private static TestFrameworkSession.Factory headlessTestFrameworkSessionFactory(
CiVisibilityRepoServices repoServices,
CiVisibilityCoverageServices.Child coverageServices,
ExecutionSettings executionSettings) {
return (String projectName, String component, Long startTime) -> {
return (String projectName,
String component,
Long startTime,
Collection<LibraryCapability> availableCapabilities) -> {
repoServices.gitDataUploader.startOrObserveGitDataUpload();

String sessionName = services.config.getCiVisibilitySessionName();
Expand All @@ -288,7 +299,8 @@ private static TestFrameworkSession.Factory headlessTestFrameworkSessionFactory(
repoServices.codeowners,
services.linesResolver,
coverageServices.coverageStoreFactory,
executionStrategy);
executionStrategy,
availableCapabilities);
};
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package datadog.trace.civisibility.config;

import datadog.trace.api.DDTags;
import datadog.trace.api.civisibility.config.LibraryCapability;
import java.util.Collection;
import java.util.EnumMap;
import java.util.HashMap;
import java.util.Map;

public abstract class LibraryCapabilityUtils {

public static final Map<LibraryCapability, String> CAPABILITY_TAG_MAP = capabilitiesTagMap();

private static Map<LibraryCapability, String> capabilitiesTagMap() {
Map<LibraryCapability, String> capabilitiesTags = new EnumMap<>(LibraryCapability.class);
capabilitiesTags.put(LibraryCapability.TIA, DDTags.LIBRARY_CAPABILITIES_TIA);
capabilitiesTags.put(LibraryCapability.EFD, DDTags.LIBRARY_CAPABILITIES_EFD);
capabilitiesTags.put(LibraryCapability.ATR, DDTags.LIBRARY_CAPABILITIES_ATR);
capabilitiesTags.put(LibraryCapability.IMPACTED, DDTags.LIBRARY_CAPABILITIES_IMPACTED_TESTS);
capabilitiesTags.put(
LibraryCapability.FAIL_FAST, DDTags.LIBRARY_CAPABILITIES_FAIL_FAST_TEST_ORDER);
capabilitiesTags.put(LibraryCapability.QUARANTINE, DDTags.LIBRARY_CAPABILITIES_QUARANTINE);
capabilitiesTags.put(LibraryCapability.DISABLED, DDTags.LIBRARY_CAPABILITIES_DISABLED);
capabilitiesTags.put(
LibraryCapability.ATTEMPT_TO_FIX, DDTags.LIBRARY_CAPABILITIES_ATTEMPT_TO_FIX);
return capabilitiesTags;
}

public static Map<LibraryCapability, Boolean> filterCapabilities(
Collection<LibraryCapability> available, Map<LibraryCapability, Boolean> capabilitiesStatus) {
Map<LibraryCapability, Boolean> capabilities = new HashMap<>(capabilitiesStatus);
capabilities.keySet().retainAll(available);
return capabilities;
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package datadog.trace.civisibility.domain;

import datadog.trace.api.civisibility.config.LibraryCapability;
import java.util.Collection;
import javax.annotation.Nullable;

/** Test session abstraction that is used by test framework instrumentations (e.g. JUnit, TestNG) */
Expand All @@ -9,6 +11,10 @@ public interface TestFrameworkSession {
TestFrameworkModule testModuleStart(String moduleName, @Nullable Long startTime);

interface Factory {
TestFrameworkSession startSession(String projectName, String component, Long startTime);
TestFrameworkSession startSession(
String projectName,
String component,
Long startTime,
Collection<LibraryCapability> availableCapabilities);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import datadog.trace.api.civisibility.CIConstants;
import datadog.trace.api.civisibility.DDTest;
import datadog.trace.api.civisibility.InstrumentationTestBridge;
import datadog.trace.api.civisibility.config.LibraryCapability;
import datadog.trace.api.civisibility.config.TestIdentifier;
import datadog.trace.api.civisibility.coverage.CoveragePerTestBridge;
import datadog.trace.api.civisibility.coverage.CoverageStore;
Expand Down Expand Up @@ -37,6 +38,7 @@
import datadog.trace.bootstrap.instrumentation.api.TagContext;
import datadog.trace.bootstrap.instrumentation.api.Tags;
import datadog.trace.civisibility.codeowners.Codeowners;
import datadog.trace.civisibility.config.LibraryCapabilityUtils;
import datadog.trace.civisibility.decorator.TestDecorator;
import datadog.trace.civisibility.source.LinesResolver;
import datadog.trace.civisibility.source.SourcePathResolver;
Expand All @@ -45,6 +47,7 @@
import java.lang.reflect.Method;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import java.util.function.Consumer;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -86,6 +89,7 @@ public TestImpl(
Codeowners codeowners,
CoverageStore.Factory coverageStoreFactory,
ExecutionResults executionResults,
Map<LibraryCapability, Boolean> libraryCapabilities,
Consumer<AgentSpan> onSpanFinish) {
this.instrumentation = instrumentation;
this.metricCollector = metricCollector;
Expand Down Expand Up @@ -144,6 +148,11 @@ public TestImpl(
span.setTag(Tags.ITR_CORRELATION_ID, itrCorrelationId);
}

for (Map.Entry<LibraryCapability, Boolean> entry : libraryCapabilities.entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

String capabilityTag = LibraryCapabilityUtils.CAPABILITY_TAG_MAP.get(entry.getKey());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

span.setTag(capabilityTag, entry.getValue());
}

testDecorator.afterStart(span);

metricCollector.add(CiVisibilityCountMetric.EVENT_CREATED, 1, instrumentation, EventType.TEST);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import datadog.trace.api.Config;
import datadog.trace.api.civisibility.DDTestSuite;
import datadog.trace.api.civisibility.config.LibraryCapability;
import datadog.trace.api.civisibility.coverage.CoverageStore;
import datadog.trace.api.civisibility.telemetry.CiVisibilityCountMetric;
import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector;
Expand All @@ -26,6 +27,7 @@
import datadog.trace.civisibility.utils.SpanUtils;
import java.lang.reflect.Method;
import java.util.Collection;
import java.util.Map;
import java.util.function.Consumer;
import javax.annotation.Nullable;
import org.slf4j.Logger;
Expand All @@ -52,6 +54,7 @@ public class TestSuiteImpl implements DDTestSuite {
private final CoverageStore.Factory coverageStoreFactory;
private final ExecutionResults executionResults;
private final boolean parallelized;
private final Map<LibraryCapability, Boolean> libraryCapabilities;
private final Consumer<AgentSpan> onSpanFinish;

public TestSuiteImpl(
Expand All @@ -72,6 +75,7 @@ public TestSuiteImpl(
LinesResolver linesResolver,
CoverageStore.Factory coverageStoreFactory,
ExecutionResults executionResults,
Map<LibraryCapability, Boolean> libraryCapabilities,
Consumer<AgentSpan> onSpanFinish) {
this.moduleSpanContext = moduleSpanContext;
this.moduleName = moduleName;
Expand All @@ -88,6 +92,7 @@ public TestSuiteImpl(
this.linesResolver = linesResolver;
this.coverageStoreFactory = coverageStoreFactory;
this.executionResults = executionResults;
this.libraryCapabilities = libraryCapabilities;
this.onSpanFinish = onSpanFinish;

AgentTracer.SpanBuilder spanBuilder =
Expand Down Expand Up @@ -259,6 +264,12 @@ public TestImpl testStart(
codeowners,
coverageStoreFactory,
executionResults,
SpanUtils.propagateCiVisibilityTagsTo(span));
libraryCapabilities,
this::propagateTestTags);
}

private void propagateTestTags(AgentSpan testSpan) {
SpanUtils.propagateCiVisibilityTags(span, testSpan);
SpanUtils.propagateLibraryCapabilities(span, testSpan);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we call propagateLibraryCapabilities from propagateCiVisibilityTags? That'd allow us to centralise the logic and have less places to update

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, let's remove the propagation altogether, as the spec only mandates that the capabilities be present in the test events

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import datadog.trace.api.Config;
import datadog.trace.api.DDTraceId;
import datadog.trace.api.civisibility.config.LibraryCapability;
import datadog.trace.api.civisibility.config.TestIdentifier;
import datadog.trace.api.civisibility.config.TestSourceData;
import datadog.trace.api.civisibility.coverage.CoverageStore;
Expand Down Expand Up @@ -30,6 +31,7 @@
import datadog.trace.civisibility.test.ExecutionResults;
import datadog.trace.civisibility.test.ExecutionStrategy;
import java.util.Collection;
import java.util.Map;
import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import javax.annotation.Nonnull;
Expand Down Expand Up @@ -60,6 +62,7 @@ public class ProxyTestModule implements TestFrameworkModule {
private final LinesResolver linesResolver;
private final CoverageStore.Factory coverageStoreFactory;
private final Collection<TestFramework> testFrameworks = ConcurrentHashMap.newKeySet();
private final Map<LibraryCapability, Boolean> libraryCapabilities;

public ProxyTestModule(
AgentSpanContext parentProcessModuleContext,
Expand All @@ -73,7 +76,8 @@ public ProxyTestModule(
LinesResolver linesResolver,
CoverageStore.Factory coverageStoreFactory,
ChildProcessCoverageReporter childProcessCoverageReporter,
SignalClient.Factory signalClientFactory) {
SignalClient.Factory signalClientFactory,
Map<LibraryCapability, Boolean> libraryCapabilities) {
this.parentProcessModuleContext = parentProcessModuleContext;
this.moduleName = moduleName;
this.executionStrategy = executionStrategy;
Expand All @@ -87,6 +91,7 @@ public ProxyTestModule(
this.codeowners = codeowners;
this.linesResolver = linesResolver;
this.coverageStoreFactory = coverageStoreFactory;
this.libraryCapabilities = libraryCapabilities;
}

@Override
Expand Down Expand Up @@ -210,6 +215,7 @@ public TestSuiteImpl testSuiteStart(
linesResolver,
coverageStoreFactory,
executionResults,
libraryCapabilities,
this::propagateTestFrameworkData);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package datadog.trace.civisibility.domain.buildsystem;

import datadog.trace.api.Config;
import datadog.trace.api.civisibility.config.LibraryCapability;
import datadog.trace.api.civisibility.coverage.CoverageStore;
import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector;
import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import datadog.trace.civisibility.codeowners.Codeowners;
import datadog.trace.civisibility.config.LibraryCapabilityUtils;
import datadog.trace.civisibility.coverage.percentage.child.ChildProcessCoverageReporter;
import datadog.trace.civisibility.decorator.TestDecorator;
import datadog.trace.civisibility.domain.TestFrameworkModule;
Expand All @@ -14,6 +16,8 @@
import datadog.trace.civisibility.source.LinesResolver;
import datadog.trace.civisibility.source.SourcePathResolver;
import datadog.trace.civisibility.test.ExecutionStrategy;
import java.util.Collection;
import java.util.Map;
import javax.annotation.Nullable;

/**
Expand All @@ -34,6 +38,7 @@ public class ProxyTestSession implements TestFrameworkSession {
private final ChildProcessCoverageReporter childProcessCoverageReporter;
private final SignalClient.Factory signalClientFactory;
private final ExecutionStrategy executionStrategy;
private final Map<LibraryCapability, Boolean> libraryCapabilities;

public ProxyTestSession(
AgentSpanContext parentProcessModuleContext,
Expand All @@ -46,7 +51,8 @@ public ProxyTestSession(
CoverageStore.Factory coverageStoreFactory,
ChildProcessCoverageReporter childProcessCoverageReporter,
SignalClient.Factory signalClientFactory,
ExecutionStrategy executionStrategy) {
ExecutionStrategy executionStrategy,
Collection<LibraryCapability> availableCapabilities) {
this.parentProcessModuleContext = parentProcessModuleContext;
this.config = config;
this.metricCollector = metricCollector;
Expand All @@ -58,6 +64,9 @@ public ProxyTestSession(
this.childProcessCoverageReporter = childProcessCoverageReporter;
this.signalClientFactory = signalClientFactory;
this.executionStrategy = executionStrategy;
this.libraryCapabilities =
LibraryCapabilityUtils.filterCapabilities(
availableCapabilities, executionStrategy.getCapabilitiesStatus());
}

@Override
Expand All @@ -82,6 +91,7 @@ public TestFrameworkModule testModuleStart(String moduleName, @Nullable Long sta
linesResolver,
coverageStoreFactory,
childProcessCoverageReporter,
signalClientFactory);
signalClientFactory,
libraryCapabilities);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import datadog.trace.api.Config;
import datadog.trace.api.DDTags;
import datadog.trace.api.civisibility.CIConstants;
import datadog.trace.api.civisibility.config.LibraryCapability;
import datadog.trace.api.civisibility.config.TestIdentifier;
import datadog.trace.api.civisibility.config.TestSourceData;
import datadog.trace.api.civisibility.coverage.CoverageStore;
Expand All @@ -27,6 +28,7 @@
import datadog.trace.civisibility.test.ExecutionResults;
import datadog.trace.civisibility.test.ExecutionStrategy;
import datadog.trace.civisibility.utils.SpanUtils;
import java.util.Map;
import java.util.function.Consumer;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
Expand All @@ -43,6 +45,7 @@ public class HeadlessTestModule extends AbstractTestModule implements TestFramew
private final CoverageStore.Factory coverageStoreFactory;
private final ExecutionStrategy executionStrategy;
private final ExecutionResults executionResults;
private final Map<LibraryCapability, Boolean> libraryCapabilities;

public HeadlessTestModule(
AgentSpanContext sessionSpanContext,
Expand All @@ -56,6 +59,7 @@ public HeadlessTestModule(
LinesResolver linesResolver,
CoverageStore.Factory coverageStoreFactory,
ExecutionStrategy executionStrategy,
Map<LibraryCapability, Boolean> libraryCapabilities,
Consumer<AgentSpan> onSpanFinish) {
super(
sessionSpanContext,
Expand All @@ -72,6 +76,7 @@ public HeadlessTestModule(
this.coverageStoreFactory = coverageStoreFactory;
this.executionStrategy = executionStrategy;
this.executionResults = new ExecutionResults();
this.libraryCapabilities = libraryCapabilities;
}

@Override
Expand Down Expand Up @@ -181,6 +186,12 @@ public TestSuiteImpl testSuiteStart(
linesResolver,
coverageStoreFactory,
executionResults,
SpanUtils.propagateCiVisibilityTagsTo(span));
libraryCapabilities,
this::propagateSuiteTags);
}

private void propagateSuiteTags(AgentSpan suiteSpan) {
SpanUtils.propagateCiVisibilityTags(span, suiteSpan);
SpanUtils.propagateLibraryCapabilities(span, suiteSpan);
}
}
Loading