Add customer-facing API for CI Visibility#5005
Conversation
…k & test command calculation in Gradle build listener
… handled in different threads
…operty to disable build tracking
… only in the project, but also in the included builds
…s stubbing of that particular class does not work on Java 9+
…rnal-api, since corresponding classes were moved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public class CIProviderGitInfoBuilder implements GitInfoBuilder { | ||
| @Override | ||
| public GitInfo build(@Nullable String repositoryPath) { | ||
| Path currentPath = repositoryPath != null ? Paths.get(repositoryPath) : null; |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
390e97f to
15012e0
Compare
nikita-tkachenko-datadog
changed the title
…te parameterized test cases run in parallel
| import java.util.Map; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| public abstract class AbstractTestDecorator extends BaseDecorator implements TestDecorator { |
There was a problem hiding this comment.
I see that all integrations have a Decorator class that extends BaseDecorator. With your change, our integrations don't use the BaseDecorator at all (nor the logic in it). I'm not sure if there's any implication in doing that, have you checked?
There was a problem hiding this comment.
There are components in the tracer that do not use decorators and instead add tags to their spans "manually" (I took inspiration from IAST, as it is similar to CI Visibility from the code organisation point of view).
I have also checked the code in BaseDecorator, and the only thing that will be missing is the ability so set spans sampling rate (which is disabled by default and has to be enabled explicitly by providing the instrumentation name - and is not applicable for CI Visibility spans anyway).
|
|
||
| @Override | ||
| protected String[] instrumentationNames() { | ||
| return new String[] {"junit", "junit-4", "junit-4-suite-events"}; |
There was a problem hiding this comment.
Related to the removal of the Decorator classes. Weren't those instrumentation names needed? I see there's logic in BaseDecorator that uses them.
There was a problem hiding this comment.
They were used to calculate the names of config properties, that were then used to determine if sampling is enabled for instrumentation and which sampling rate should be used.
We do not apply sampling to CI Visibility spans as it makes little sense from the product point of view.
| final String frameworkVersion) { | ||
| DDTestModule testModule = getTestModule(sessionKey, moduleName); | ||
| testModule.setTag(Tags.TEST_FRAMEWORK, frameworkName); | ||
| testModule.setTag(Tags.TEST_FRAMEWORK_VERSION, frameworkVersion); |
There was a problem hiding this comment.
Why do we have both "detected" and "start"? This seems more complex to me that just passing those two tags and setting them in the "start" function. I'm not asking you to change it, I'm just curious to know the reasons :)
There was a problem hiding this comment.
I agree that it looks somewhat clumsy from the API point of view, but there's a reason for it :)
In Gradle we emit the session start event as soon as the build begins. Then follows a (sometimes lengthy) configuration phase when projects are evaluated, and only at the end of that phase we can examine the list of dependencies to determine the test frameworks.
If we fire the session start event after projects evaluation finishes, our total session duration will be skewed since we won't be counting the configuration phase.
Besides, it is possible to fail during configuration phase - before projects are evaluated - in which case we'd have to emit a failed session span.
Having two different events seemed like a simple way to cover the two cases above.
| protected String runtimeName() { | ||
| return System.getProperty("java.runtime.name"); | ||
| } |
There was a problem hiding this comment.
Not related to this PR, but we could cache those properties in local strings instead of calling getProperty each time.
There was a problem hiding this comment.
Agree, could be worth doing.
| minimumInstructionCoverage = 0.8 | ||
|
|
||
| excludedClassesCoverage += ["datadog.trace.civisibility.CiVisibilitySystem", "datadog.trace.civisibility.git.GitObject",] | ||
| excludedClassesCoverage += [ |
There was a problem hiding this comment.
Do we have to remember to add every class we create here manually? Can't we use wildcards? What's this for?
There was a problem hiding this comment.
There's a logic that evaluates code coverage stats during CI tests and fails the build if coverage percentage criteria are not met.
This list is needed to disable coverage check for specific classes (e.g. DTOs, or those that are covered with integration/smoke tests).
Wildcards are supported, but I think it's already bad enough without them :) at least now every entry has to be added consciously.
3 participants
What Does This Do
Adds an API to create CI Visibility events (session, module, test suite, test case) programmatically.
The API is exposed to the customers.
Motivation
The API is needed to support the needs of customers who are using either test frameworks that we do not instrument, or ad-hoc testing logic.
Such customers would be able to manually demarcate the start and end of their tests, and to supply additional test metadata.
Additional Notes
In Scope of this PR existing CI Visibility logic was refactored to make test framework instrumentations use the same domain objects that are used by the manual API.