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): flesh out support for ITR in manual API #8792

Merged

Conversation

romainkomorndatadog
Copy link
Collaborator

@romainkomorndatadog romainkomorndatadog commented Mar 27, 2024

Adds support to the manual API for ITR-related activity:

  • counting skippable/skipped items
  • unskippable and forced run tags
  • adds ITR-related settings to CIVisibilitySessionSettings
  • only settings ITR-related tags when ITR is enabled
  • ensuring that skipped counts only counts suites when in suite-skipping mode (CIVIS-8754)

CIITRMixin is introduced for the external API, since both suites and tests support ITR.

Some other minor changes include:

  • use class name instead of base class names (shorter and nicer)
  • shuffling some args namedtuple definitions around for cosmetic reasons
  • test updates to properly simulate suite-level skipping

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 romainkomorndatadog added the changelog/no-changelog A changelog entry is not required for this PR. label Mar 27, 2024
@romainkomorndatadog romainkomorndatadog self-assigned this Mar 27, 2024
@datadog-dd-trace-py-rkomorn
Copy link

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

Datadog Report

Branch report: romain.komorn/CIVIS-2943/itr_fetching_tests
Commit report: 7cb2730
Test service: dd-trace-py

✅ 0 Failed, 36249 Passed, 18 Skipped, 11m 21.13s Wall Time

@pr-commenter
Copy link

pr-commenter bot commented Mar 28, 2024

Benchmarks

Benchmark execution time: 2024-03-29 08:44:02

Comparing candidate commit 868f280 in PR branch romain.komorn/CIVIS-2943/itr_fetching_tests with baseline commit 8ab2d72 in branch main.

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

scenario:httppropagationextract-large_header_no_matches

  • 🟩 max_rss_usage [-746.562KB; -484.286KB] or [-3.411%; -2.213%]

scenario:httppropagationextract-none_propagation_style

  • 🟩 max_rss_usage [-1027.047KB; -762.905KB] or [-4.695%; -3.487%]

scenario:httppropagationextract-wsgi_large_header_no_matches

  • 🟩 max_rss_usage [-750.469KB; -676.987KB] or [-3.432%; -3.096%]

scenario:httppropagationextract-wsgi_medium_header_no_matches

  • 🟩 max_rss_usage [-764.930KB; -689.559KB] or [-3.499%; -3.154%]

Copy link
Contributor

@erikayasuda erikayasuda left a comment

Choose a reason for hiding this comment

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

A couple of non-blocking questions for my own understanding. Otherwise LGTM!

ddtrace/internal/ci_visibility/recorder.py Show resolved Hide resolved
ddtrace/internal/ci_visibility/api/ci_base.py Show resolved Hide resolved
@romainkomorndatadog romainkomorndatadog enabled auto-merge (squash) March 29, 2024 07:41
@romainkomorndatadog romainkomorndatadog merged commit 2ac11e5 into main Apr 2, 2024
88 of 89 checks passed
@romainkomorndatadog romainkomorndatadog deleted the romain.komorn/CIVIS-2943/itr_fetching_tests branch April 2, 2024 08:40
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.

3 participants