Skip to content

chore(telemetry): report tested integration versions #6853

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

Merged
merged 46 commits into from
Oct 16, 2023

Conversation

wconti27
Copy link
Contributor

@wconti27 wconti27 commented Sep 7, 2023

Motivation

With this PR we want to add a method to the generated patch for all contribs that will emit a telemetry payload describing the tested integration and its tested version. The goal of this PR is to get a list of all integrations tested during CI, along with the range of versions tested, which will be reported to metabase. The reasoning for adding another testcase instead of simply using information available in metabase from customer telemetry on what integrations are used is as follows:

  • we want the ability to know if an integration @ a specific version is being supported and actively tested, but not in use by any customers; using customer telemetry would therefore not cover this case and not allow us to deprecate support for unused integrations.

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed. If no release note is required, add label changelog/no-changelog.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer has explicitly 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
  • If this PR 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.
  • This PR doesn't touch any of that.

@wconti27 wconti27 requested a review from mabdinur September 7, 2023 13:09
Copy link
Contributor

@mabdinur mabdinur left a comment

Choose a reason for hiding this comment

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

Although this approach currently works it uses the instrumentation telemetry client in an unsupported way. To support python3.12 we recently changed when/how telemtry events are sent. This change will prevent integrations will not be sent in test runs: #6859. We can hack around this issue but I don't think this is the best approach.

Can we get the tested integration versions by parsing the venvs in https://github.com/DataDog/dd-trace-py/blob/1.x/riotfile.py? This way we can collect and submit the integration data to the test agent once. We can also make this nightly job on the 1.x branch so this data is not collected on every ci run.

@majorgreys majorgreys changed the base branch from 1.x to 2.x September 18, 2023 17:53
@wconti27 wconti27 changed the title Conti/test integration versions branch 2 dev: report tested integration versions Oct 2, 2023
@pr-commenter
Copy link

pr-commenter bot commented Oct 2, 2023

Benchmarks

Benchmark execution time: 2023-10-16 14:18:22

Comparing candidate commit b2d4ce4 in PR branch conti/test-integration-versions-branch-2 with baseline commit 7ec041b in branch 2.x.

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

Copy link
Contributor

@mabdinur mabdinur left a comment

Choose a reason for hiding this comment

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

This is beyond the scope of this PR but some of the modules have the name test_integrationname_patch_generated. We need to remove the generated part of the name and make sure the patch test is actually making the expected assertions.

@wconti27 wconti27 marked this pull request as ready for review October 16, 2023 13:20
@wconti27 wconti27 requested review from a team as code owners October 16, 2023 13:20
@mabdinur mabdinur changed the title dev: report tested integration versions chore(telemetry): report tested integration versions Oct 16, 2023
@wconti27 wconti27 enabled auto-merge (squash) October 16, 2023 13:33
@wconti27 wconti27 added the changelog/no-changelog A changelog entry is not required for this PR. label Oct 16, 2023
@wconti27 wconti27 self-assigned this Oct 16, 2023
@wconti27 wconti27 merged commit 551c8d5 into 2.x Oct 16, 2023
@wconti27 wconti27 deleted the conti/test-integration-versions-branch-2 branch October 16, 2023 14:58
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