Skip to content

Add tests for remote disablement of APM#2039

Merged
emmettbutler merged 7 commits intomainfrom
emmett.butler/tracing-enabled-rc-capability
Jan 25, 2024
Merged

Add tests for remote disablement of APM#2039
emmettbutler merged 7 commits intomainfrom
emmett.butler/tracing-enabled-rc-capability

Conversation

@emmettbutler
Copy link
Contributor

@emmettbutler emmettbutler commented Jan 19, 2024

Motivation & Changes

In support of the Python implementation of runtime tracing un-instrumentation in DataDog/dd-trace-py#8080, I've implemented a simple black box test of the functionality described in the relevant product briefs.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • Relevant label (run-parametric-scenario, run-profiling-scenario...) are presents
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
    • To R&P team: locally build and push the image to hub.docker.com
  • A scenario is added (or removed)?
    • Get a review from R&P team
    • Once merged, add (or remove) it in system-test-dasboard nightly

@emmettbutler emmettbutler requested a review from a team as a code owner January 19, 2024 18:34
@emmettbutler emmettbutler marked this pull request as draft January 19, 2024 18:34
@emmettbutler emmettbutler marked this pull request as ready for review January 19, 2024 18:48
Co-authored-by: Kyle Verhoog <kyle@verhoog.ca>
Copy link
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

New test is failing.

Copy link
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

AGTM

Though, I did not reviwed the test logic, it may be a good idea to get a review from someone familiar with the tested feature.

@Kyle-Verhoog Kyle-Verhoog changed the title test the tracing_enabled remote config flag Add tests for remote disablement of APM Jan 24, 2024
@emmettbutler emmettbutler requested review from a team as code owners January 25, 2024 21:11
@emmettbutler emmettbutler merged commit 89b86d1 into main Jan 25, 2024
@emmettbutler emmettbutler deleted the emmett.butler/tracing-enabled-rc-capability branch January 25, 2024 21:13
@cbeauchesne
Copy link
Collaborator

@emmettbutler for next PR, could you check CI before merging ? this PR has been merged while the CI was failing on the new test, and in consequence, CI of dd-trace-java, dd-trace-rb and dd-trace-php was broken (tks @robertomonteromiguel for #2060).

For more context, we did not added strict rules on this repo, to keep our agility (flakiness, and also failures related to something else happens quite often), so as long as a single approval at any moment exists, we can merge. But it requires an extra attention when the CI is red.

emmettbutler added a commit to DataDog/dd-trace-py that referenced this pull request Feb 1, 2024
…#8080)

This pull request makes `Tracer.enabled` configurable via remote.
Semantically this configuration is a one-way switch: it can be remotely
set to False, but once it's False it cannot be remotely set to True.
When False, it can be set to True via code or environment variable.

There isn't much risk in this change because it's a no-op until the UI
starts sending the relevant remote configuration flag.

The remote config backend change supporting this feature is dd-go PR
118200.
The system-tests change validating this new functionality is
DataDog/system-tests#2039.

## 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] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [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] Check against client capabilities documentation
- [x] system-tests added here
https://github.com/DataDog/system-tests/blob/37f0b61616884cbb8dd553415409c1ad0152ad98/tests/parametric/test_dynamic_configuration.py

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly 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)
- [x] 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`.
- [x] This PR doesn't touch any of that.

APMON-578

---------

Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants