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

chore(ci_visibility): add ITR coverage support to manual API #8768

Merged

Conversation

romainkomorndatadog
Copy link
Collaborator

@romainkomorndatadog romainkomorndatadog commented Mar 26, 2024

Adds support for submitting coverage data using the CI Visibility manual API.

The CICoverageData class is used as a container. One of the features it will enable is adding coverage data "a piece at a time" instead of one-shotting the entire coverage data (which is still possible).

This will be useful for cases where tests don't run in-order within suites (which is possible in cases like pytest-xdist).

Two worthwhile mentions compared to existing approach to coverage:

  • paths are stored using pathlib.Path rather than strings
  • paths are stored as absolute paths (by calling .absolute() on all given Paths objects)

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.
  • If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

romainkomorndatadog and others added 6 commits March 22, 2024 17:01
ASM: This fix resolves an issue where django login failure events may
send wrong information of user existence.

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
…s exceeded (#8739)

Prior to this change DSM Processor failures to contact the agent were
logged with `log.error(...)`, even though they might be retried and be
successful.

After this change, those messages are now _debug level_. To make sure
the message still appears, the enclosing try/catch statement has been
updated to include the stack trace in the error message.

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [ ] Title is accurate
- [ ] All changes are related to the pull request's stated goal
- [ ] Description motivates each change
- [ ] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [ ] Testing strategy adequately addresses listed risks
- [ ] Change is maintainable (easy to change, telemetry, documentation)
- [ ] Release note makes sense to a user of the library
- [ ] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [ ] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
This change partially addresses #7227 by adding release notes to the
changelog file during the release process. This was tested first with
`DRY_RUN=1` and later without that flag during the 2.6.9 release
process. The pull request created during that test is #8737.

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
@romainkomorndatadog romainkomorndatadog added the changelog/no-changelog A changelog entry is not required for this PR. label Mar 26, 2024
@romainkomorndatadog romainkomorndatadog self-assigned this Mar 26, 2024
@romainkomorndatadog romainkomorndatadog changed the title chore(ci_visibility): CI Visibility manual API: add ITR coverage support to manual chore(ci_visibility): add ITR coverage support to manual API Mar 26, 2024
@romainkomorndatadog
Copy link
Collaborator Author

Worth noting here, there's a known (by me) issue here where suite-level coverage is not being added.

I'm not addressing this because I'm working on another change on top of this one that refactors that part of the code.

@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Mar 26, 2024

Datadog Report

Branch report: romain.komorn/CIVIS-2943/add_coverage_support
Commit report: cb16903
Test service: dd-trace-py

✅ 0 Failed, 36226 Passed, 18 Skipped, 9m 45.37s Wall Time

@pr-commenter
Copy link

pr-commenter bot commented Mar 26, 2024

Benchmarks

Benchmark execution time: 2024-03-26 15:42:46

Comparing candidate commit 3fe1db5 in PR branch romain.komorn/CIVIS-2943/add_coverage_support with baseline commit 7ed8fbc in branch main.

Found 4 performance improvements and 3 performance regressions! Performance is the same for 194 metrics, 9 unstable metrics.

scenario:flasksimple-appsec-get

  • 🟥 execution_time [+241.624µs; +300.735µs] or [+3.858%; +4.802%]

scenario:flasksimple-appsec-telemetry

  • 🟥 execution_time [+234.096µs; +297.340µs] or [+3.737%; +4.747%]

scenario:flasksimple-tracer

  • 🟥 execution_time [+252.137µs; +296.635µs] or [+4.026%; +4.736%]

scenario:httppropagationextract-b3_headers

  • 🟩 max_rss_usage [-747.437KB; -671.008KB] or [-3.418%; -3.068%]

scenario:httppropagationextract-large_header_no_matches

  • 🟩 max_rss_usage [-777.876KB; -509.906KB] or [-3.561%; -2.335%]

scenario:httppropagationextract-medium_header_no_matches

  • 🟩 max_rss_usage [-788.061KB; -636.527KB] or [-3.603%; -2.910%]

scenario:httppropagationextract-wsgi_valid_headers_basic

  • 🟩 max_rss_usage [-839.789KB; -645.011KB] or [-3.840%; -2.950%]

@romainkomorndatadog romainkomorndatadog enabled auto-merge (squash) March 27, 2024 14:41
@romainkomorndatadog romainkomorndatadog merged commit a04d66b into main Mar 27, 2024
67 of 68 checks passed
@romainkomorndatadog romainkomorndatadog deleted the romain.komorn/CIVIS-2943/add_coverage_support branch March 27, 2024 15:12
christophe-papazian pushed a commit that referenced this pull request Mar 29, 2024
Adds support for submitting coverage data using the CI Visibility manual
API.

The `CICoverageData` class is used as a container. One of the features
it will enable is adding coverage data "a piece at a time" instead of
one-shotting the entire coverage data (which is still possible).

This will be useful for cases where tests don't run in-order within
suites (which is possible in cases like `pytest-xdist`).

Two worthwhile mentions compared to existing approach to coverage:
- paths are stored using `pathlib.Path` rather than strings
- paths are stored as absolute paths (by calling `.absolute()` on all
given `Paths` objects)

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Federico Mon <federico.mon@datadoghq.com>
Co-authored-by: Teague Bick <teague.bick@datadoghq.com>
Co-authored-by: Emmett Butler <723615+emmettbutler@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants