feat(tracer): collect and report multiple service names #6929

christophe-papazian · 2023-09-18T13:45:05Z

Implementation of RFC "Service Names - Handling Inferred (Fake) Services"

add a multiprocessing queue to config
all workers will push discovered sub-services names on the queue
main thread will pull all names and send them to agent using Remote Config
add two unit tests and update one RC test
it will also be validated by system tests
update and activate remote config tests for extra services system-tests#1623

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

…and-report-multiple-service-names

pr-commenter · 2023-09-20T15:02:14Z

Benchmarks

Benchmark execution time: 2023-09-26 21:51:45

Comparing candidate commit d55a4da in PR branch christophe-papazian/APPSEC-11098-rcm-collect-and-report-multiple-service-names with baseline commit 518365c in branch 2.x.

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

scenario:otelspan-add-metrics

🟩 max_rss_usage [-2.322MB; -2.148MB] or [-5.410%; -5.005%]

scenario:otelspan-add-tags

🟩 max_rss_usage [-2.191MB; -2.027MB] or [-5.095%; -4.714%]

scenario:otelspan-start-finish-telemetry

🟥 max_rss_usage [+567.985KB; +712.834KB] or [+2.106%; +2.643%]

scenario:span-add-tags

🟥 max_rss_usage [+1.692MB; +1.849MB] or [+3.972%; +4.340%]

…PSEC-11098-rcm-collect-and-report-multiple-service-names

releasenotes/notes/collect_extra_service_names-453190f684d62070.yaml

…and-report-multiple-service-names

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.

Implementation of RFC "Service Names - Handling Inferred (Fake) Services" - add a multiprocessing queue to config - all workers will push discovered sub-services names on the queue - main thread will pull all names and send them to agent using Remote Config - add two unit tests and update one RC test - it will also be validated by system tests DataDog/system-tests#1623 ## 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.

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.

#6929 introduced a bug that makes remote config spawns an infinite number of processes, but only on mac osx with some specific settings. This is related to the different default settings of the multiprocessing python module (not the same on unix and osx). This fix the problem. ## 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. - [ ] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [ ] 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. Co-authored-by: Gabriele N. Tornetta <P403n1x87@users.noreply.github.com>

#6929 introduced a bug as multiprocessing.Queue is not supported on AWS lambda platform. This fix prevents the use of the Queue on aws lambda, as this is used for a RC only feature, and lambda does not use RC. ## 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. --------- Co-authored-by: Zachary Groves <32471391+ZStriker19@users.noreply.github.com> Co-authored-by: ZStriker19 <zach.groves@datadoghq.com> (cherry picked from commit 1656e7d)

#6929 introduced a bug as multiprocessing.Queue is not supported on AWS lambda platform. This fix prevents the use of the Queue on aws lambda, as this is used for a RC only feature, and lambda does not use RC. ## 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. --------- Co-authored-by: Zachary Groves <32471391+ZStriker19@users.noreply.github.com> Co-authored-by: ZStriker19 <zach.groves@datadoghq.com>

…7692) Backport 1656e7d from #7612 to 2.2. #6929 introduced a bug as multiprocessing.Queue is not supported on AWS lambda platform. This fix prevents the use of the Queue on aws lambda, as this is used for a RC only feature, and lambda does not use RC. ## 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. Co-authored-by: Christophe Papazian <114495376+christophe-papazian@users.noreply.github.com>

christophe-papazian added 3 commits September 15, 2023 17:34

add extra service mechanism

65848f6

remove tracer test

5c5a401

update rc tests

d8146f5

majorgreys changed the base branch from 1.x to 2.x September 18, 2023 17:53

Merge branch '2.x' into christophe-papazian/APPSEC-11098-rcm-collect-…

5e1728d

…and-report-multiple-service-names

christophe-papazian added 15 commits September 22, 2023 09:55

Merge remote-tracking branch 'origin/2.x' into christophe-papazian/AP…

dbcd1ea

…PSEC-11098-rcm-collect-and-report-multiple-service-names

improve test

c79d329

simple queue mechanism

e77a1e3

fixes

c6a238e

fixes

cbffdcc

nosec

99c58b2

fix imports and tests

e1001cc

add traceback info

3741f48

try fixing circular import in processor

0b6dc92

catch all in waf

aeaac74

Merge remote-tracking branch 'origin/2.x' into christophe-papazian/AP…

6421c36

…PSEC-11098-rcm-collect-and-report-multiple-service-names

add guard for linux loading

d6fe4de

dynamic update of tracer config

1168d3d

merge 2.x

dc9971d

revert tracer changes

5cbcf72

christophe-papazian changed the title ~~Christophe papazian/appsec 11098 rcm collect and report multiple service names~~ feat(tracer): collect and report multiple service names Sep 26, 2023

christophe-papazian added 2 commits September 26, 2023 10:32

add types and make fields private

2315bba

release note

4e5b212

christophe-papazian added the enhancement label Sep 26, 2023

christophe-papazian added 2 commits September 26, 2023 10:54

fix type annotation not compatible with python 3.7

5d28cf4

test update

26d3041

christophe-papazian marked this pull request as ready for review September 26, 2023 11:11

christophe-papazian requested review from a team as code owners September 26, 2023 11:11

emmettbutler self-requested a review September 26, 2023 15:42

emmettbutler previously approved these changes Sep 26, 2023

View reviewed changes

releasenotes/notes/collect_extra_service_names-453190f684d62070.yaml Outdated Show resolved Hide resolved

avara1986 previously approved these changes Sep 26, 2023

View reviewed changes

release note improvement

9407b05

christophe-papazian dismissed stale reviews from avara1986 and emmettbutler via 9407b05 September 26, 2023 16:37

christophe-papazian requested a review from emmettbutler September 26, 2023 16:38

christophe-papazian enabled auto-merge (squash) September 26, 2023 16:38

emmettbutler approved these changes Sep 26, 2023

View reviewed changes

emmettbutler added 2 commits September 26, 2023 13:28

Merge branch '2.x' into christophe-papazian/APPSEC-11098-rcm-collect-…

61459be

…and-report-multiple-service-names

Merge branch '2.x' into christophe-papazian/APPSEC-11098-rcm-collect-…

d55a4da

…and-report-multiple-service-names

christophe-papazian merged commit 4cb41f0 into 2.x Sep 27, 2023
134 checks passed

christophe-papazian deleted the christophe-papazian/APPSEC-11098-rcm-collect-and-report-multiple-service-names branch September 27, 2023 07:39

github-actions bot added this to the v2.1.0 milestone Sep 27, 2023

christophe-papazian mentioned this pull request Sep 27, 2023

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

Merged

18 tasks

christophe-papazian mentioned this pull request Sep 28, 2023

chore(tracer): defuse osx thread bomb #7096

Merged

18 tasks

majorgreys mentioned this pull request Oct 3, 2023

chore(asm): fix win32 fork usage error #7126

Merged

18 tasks

hfern mentioned this pull request Oct 17, 2023

AWS Lambda gives OSError: [Errno 38] Function not implemented with ddtrace 2.1.0 #7251

Closed

christophe-papazian mentioned this pull request Nov 15, 2023

fix(tracer): fix multiprocessing queue on aws lambda #7612

Merged

18 tasks

github-actions bot mentioned this pull request Nov 20, 2023

fix(tracer): fix multiprocessing queue on aws lambda [backport 2.2] #7692

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracer): collect and report multiple service names #6929

feat(tracer): collect and report multiple service names #6929

christophe-papazian commented Sep 18, 2023 •

edited by emmettbutler

Loading

pr-commenter bot commented Sep 20, 2023 •

edited

Loading

feat(tracer): collect and report multiple service names #6929

feat(tracer): collect and report multiple service names #6929

Conversation

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

Checklist

Reviewer Checklist

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

Benchmarks

scenario:otelspan-add-metrics

scenario:otelspan-add-tags

scenario:otelspan-start-finish-telemetry

scenario:span-add-tags

christophe-papazian commented Sep 18, 2023 •

edited by emmettbutler

Loading

pr-commenter bot commented Sep 20, 2023 •

edited

Loading