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.

Sign up for GitHub

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

Jump to bottom

chore(tracer): limit memory usage for extra services reports #7060

Merged
merged 7 commits into from
Sep 27, 2023
Merged

chore(tracer): limit memory usage for extra services reports #7060

merged 7 commits into from
Sep 27, 2023

Conversation

christophe-papazian
Copy link
Contributor

@christophe-papazian christophe-papazian commented Sep 27, 2023 •
edited by emmettbutler
Loading

Small guard for unreleased feature merged in #6929

  • add check to not fill the queue if remote config is not enabled
  • add max size to the queue to avoid any memory leak in case of a remote config malfunction that would lead to never empty the queue. If the queue is full, new service names will be silently discarded.
  • improve flask unit test of the feature to avoid flakiness due to thread concurrency.

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.

@christophe-papazian christophe-papazian added the changelog/no-changelog A changelog entry is not required for this PR. label Sep 27, 2023
@pr-commenter
Copy link

pr-commenter bot commented Sep 27, 2023 •
edited
Loading

Benchmarks

Benchmark execution time: 2023-09-27 12:14:07

Comparing candidate commit ebe6b22 in PR branch christophe-papazian/APPSEC-11098_limit_memory with baseline commit 17f2c7b in branch 2.x.

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

@christophe-papazian christophe-papazian marked this pull request as ready for review September 27, 2023 11:50
@christophe-papazian christophe-papazian requested a review from a team as a code owner September 27, 2023 11:50
@christophe-papazian christophe-papazian requested review from majorgreys and removed request for majorgreys September 27, 2023 11:50
@christophe-papazian christophe-papazian merged commit f00acf8 into 2.x Sep 27, 2023
128 of 129 checks passed
@christophe-papazian christophe-papazian deleted the christophe-papazian/APPSEC-11098_limit_memory branch September 27, 2023 13:50
@github-actions github-actions bot added this to the v2.1.0 milestone Sep 27, 2023
@emmettbutler emmettbutler mentioned this pull request Sep 27, 2023
Merged
18 tasks
emmettbutler added a commit that referenced this pull request Sep 27, 2023
@emmettbutler
Revert "chore(tracer): limit memory usage for extra services reports (#…
7a57601 
…7060)"

This reverts commit f00acf8.
@emmettbutler emmettbutler mentioned this pull request Sep 27, 2023
Merged
1 task
emmettbutler added a commit that referenced this pull request Sep 27, 2023
@emmettbutler
chore(tracer): revert "limit memory usage for extra services reports" (…
c6808b2 
…#7074)

Reverts #7060

- [x] foobar
@emmettbutler
Copy link
Collaborator

This change was reverted because it caused a test to fail on trunk.

tabgok pushed a commit that referenced this pull request Sep 28, 2023
@christophe-papazian @tabgok
chore(tracer): limit memory usage for extra services reports (#7060)
94bdf9a 
Small guard for unreleased feature merged in
#6929

- add check to not fill the queue if remote config is not enabled
- add max size to the queue to avoid any memory leak in case of a remote
config malfunction that would lead to never empty the queue. If the
queue is full, new service names will be silently discarded.
- improve flask unit test of the feature to avoid flakiness due to
thread concurrency.

## 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))

## 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.
tabgok pushed a commit that referenced this pull request Sep 28, 2023
@emmettbutler @tabgok
chore(tracer): revert "limit memory usage for extra services reports" (…
768305f 
…#7074)

Reverts #7060

- [x] foobar
emmettbutler added a commit that referenced this pull request Oct 2, 2023
@emmettbutler
ci: add settings/config.py to core in suitespec (#7071)
bcd04e4 
This change fixes an overlooked file inclusion in the suitespec
discovered via #7060.

## 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))

## 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.
emmettbutler added a commit that referenced this pull request Oct 10, 2023
@emmettbutler
chore(tracer): revert "limit memory usage for extra services reports" (…
e2924c3 
…#7074)

Reverts #7060

- [x] foobar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmettbutler emmettbutler emmettbutler approved these changes

Assignees
No one assigned
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@christophe-papazian @emmettbutler