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(profiling): add non-running tests for stack v2 [backport 2.7] #8824

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

github-actions[bot]
Copy link

@github-actions github-actions bot commented Apr 1, 2024

Backport eb635dd from #8576 to 2.7.

This PR creates a harness for independently building and running the stack v2 code. I did it this way for a few reasons

  • Don't want to add unnecessary overhead to our normal test process
  • Want to run the build for these native components under multiple static analysis tools
  • Want to run some of the tests for these components under sanitizers?
  • But also want to do exhaustive testing of the interfaces, since we've had a few regressions here

This PR includes both the tests and the fixups they demand. A separate PR will add a CI job to execute these (or will add it to a pre-existing job, TBD).

There IS some dead code in here, since we do provide the recipe for getting and running Infer. During testing, I found that I couldn't reliably get infer to find stdlib headers, which completely broke the quality of its output, so it won't actually be used. At the same time, I think we might want to use it someday, and the setup is a little fiddly, so I'm including it here.

I'm recommending a backport to 2.7 for this because the latest 2.7 releases make this feature available, but the implementation is subtly (and sometimes not-so-subtly) broken without the fixups here. Since this feature is so isolated (I'm the only one backporting anything related!), this feels safe...ish.

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

This PR creates a harness for independently building and running the
stack v2 code. I did it this way for a few reasons

* Don't want to add unnecessary overhead to our normal test process
* Want to run the build for these native components under multiple
static analysis tools
* Want to run some of the tests for these components under sanitizers?
* But also want to do exhaustive testing of the interfaces, since we've
had a few regressions here

This PR includes both the tests and the fixups they demand. A separate
PR will add a CI job to execute these (or will add it to a pre-existing
job, TBD).

There IS some dead code in here, since we do provide the recipe for
getting and running Infer. During testing, I found that I couldn't
reliably get infer to find stdlib headers, which completely broke the
quality of its output, so it won't actually be used. At the same time, I
think we might want to use it someday, and the setup is a little fiddly,
so I'm including it here.

I'm recommending a backport to 2.7 for this because the latest 2.7
releases make this feature available, but the implementation is subtly
(and sometimes not-so-subtly) broken without the fixups here. Since this
feature is so isolated (I'm the only one backporting anything related!),
this feels safe...ish.

## 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: sanchda <sanchda@users.noreply.github.com>
Co-authored-by: Gabriele N. Tornetta <P403n1x87@users.noreply.github.com>
(cherry picked from commit eb635dd)
@github-actions github-actions bot requested review from a team as code owners April 1, 2024 18:35
@github-actions github-actions bot added the changelog/no-changelog A changelog entry is not required for this PR. label Apr 1, 2024
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Apr 1, 2024

Datadog Report

Branch report: backport-8576-to-2.7
Commit report: a12beee
Test service: dd-trace-py

✅ 0 Failed, 170987 Passed, 1076 Skipped, 11h 22m 58.45s Total duration (30m 38.51s time saved)

@pr-commenter
Copy link

pr-commenter bot commented Apr 1, 2024

Benchmarks

Benchmark execution time: 2024-04-01 19:38:59

Comparing candidate commit a12beee in PR branch backport-8576-to-2.7 with baseline commit 623a2af in branch 2.7.

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

scenario:span-add-metrics

  • 🟥 max_rss_usage [+25.828MB; +25.896MB] or [+83.435%; +83.657%]

@sanchda sanchda enabled auto-merge (squash) April 2, 2024 13:20
@sanchda sanchda merged commit 9a811d9 into 2.7 Apr 2, 2024
175 of 176 checks passed
@sanchda sanchda deleted the backport-8576-to-2.7 branch April 2, 2024 13:21
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

2 participants