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

Include the extra services found in spans in the remote config client request payloads [APPSEC-9476] #3635

Merged
merged 10 commits into from
Sep 28, 2023

Conversation

iunanua
Copy link
Contributor

@iunanua iunanua commented Sep 15, 2023

What does this PR do?

  • Includes the new module service-naming/extra-services as a repository for extra service names.
  • Defines the new environment variable DD_EXTRA_SERVICES with which is possible to define a comma-separated list of possible service extra names that are not detected automatically and the new property extraServices in TracerOptions
  • When formatting the spans, registers the services names that do not match with the tracer service name in the extra-services repo
  • Sends the extra service names in each remote config client request in the state.client.client_tracer.extra_services property of the payload

Motivation

The backend needs to know all the different services names created automatically by tracer plugins and components and the ones created by the user via SDK for the Remote Config to work correctly.

Plugin Checklist

Additional Notes

@iunanua iunanua changed the title Include in the remote config client request the extra services found in spans Include the extra services found in spans in the remote config client request payloads Sep 15, 2023
@github-actions
Copy link

github-actions bot commented Sep 15, 2023

Overall package size

Self size: 5.23 MB
Deduped: 60.72 MB
No deduping: 60.89 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 1.5.0 14.86 MB 14.86 MB
@datadog/native-appsec 4.0.0 14.83 MB 14.83 MB
@datadog/pprof 3.2.0 10.8 MB 11.64 MB
protobufjs 7.2.4 2.74 MB 6.52 MB
@datadog/native-iast-rewriter 2.1.3 2.23 MB 2.32 MB
@opentelemetry/core 1.14.0 872.87 kB 1.47 MB
@datadog/native-metrics 2.0.0 898.77 kB 1.3 MB
@opentelemetry/api 1.4.1 780.32 kB 780.32 kB
import-in-the-middle 1.4.2 41.4 kB 704.79 kB
msgpack-lite 0.1.26 201.16 kB 281.59 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.4 93.4 kB 123.8 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ipaddr.js 2.1.0 60.23 kB 60.23 kB
ignore 5.2.4 51.22 kB 51.22 kB
int64-buffer 0.1.10 49.18 kB 49.18 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
lodash.uniq 4.5.0 25.01 kB 25.01 kB
limiter 1.1.5 23.17 kB 23.17 kB
retry 0.13.1 18.85 kB 18.85 kB
lodash.kebabcase 4.1.1 17.75 kB 17.75 kB
node-abort-controller 3.1.1 16.89 kB 16.89 kB
lodash.pick 4.4.0 16.33 kB 16.33 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
diagnostics_channel 1.1.0 7.07 kB 7.07 kB
path-to-regexp 0.1.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
methods 1.1.2 5.29 kB 5.29 kB
module-details-from-path 1.0.3 4.47 kB 4.47 kB

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #3635 (bd77c29) into master (afc84ea) will increase coverage by 0.19%.
Report is 19 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3635      +/-   ##
==========================================
+ Coverage   84.59%   84.78%   +0.19%     
==========================================
  Files         217      220       +3     
  Lines        8777     8973     +196     
  Branches       33       33              
==========================================
+ Hits         7425     7608     +183     
- Misses       1352     1365      +13     
Files Coverage Δ
...kages/dd-trace/src/appsec/remote_config/manager.js 100.00% <100.00%> (ø)
packages/dd-trace/src/format.js 99.05% <100.00%> (+0.01%) ⬆️
...ages/dd-trace/src/service-naming/extra-services.js 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pr-commenter
Copy link

pr-commenter bot commented Sep 15, 2023

Benchmarks

Benchmark execution time: 2023-09-27 09:12:54

Comparing candidate commit bd77c29 in PR branch igor/rc-extra-services with baseline commit afc84ea in branch master.

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

scenario:plugin-graphql-with-depth-and-collapse-off-18

  • 🟥 max_rss_usage [+127.343KB; +185.373KB] or [+10.942%; +15.928%]

scenario:plugin-graphql-with-depth-and-collapse-on-18

  • 🟥 max_rss_usage [+105.909KB; +169.919KB] or [+12.858%; +20.629%]

scenario:plugin-graphql-with-depth-off-18

  • 🟥 max_rss_usage [+115.631KB; +138.789KB] or [+14.051%; +16.865%]

scenario:plugin-graphql-with-depth-on-max-18

  • 🟥 max_rss_usage [+125.852KB; +141.812KB] or [+15.269%; +17.205%]

@iunanua iunanua marked this pull request as ready for review September 19, 2023 06:51
@iunanua iunanua requested review from a team as code owners September 19, 2023 06:51
simon-id
simon-id previously approved these changes Sep 22, 2023
Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

If I understand this properly, this is not an actual configuration, but a workaround for something that we should be able to detect automatically? Let's not add a configuration option if it's not meant to configure something.

@iunanua
Copy link
Contributor Author

iunanua commented Sep 26, 2023

If I understand this properly, this is not an actual configuration, but a workaround for something that we should be able to detect automatically? Let's not add a configuration option if it's not meant to configure something.

I've removed the configuration option but kept the environment variable. Do you agree or would you also remove the env var?

@iunanua
Copy link
Contributor Author

iunanua commented Sep 27, 2023

Removed also the environment variable.

Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

This could probably be done more cleanly with diagnostics channel, but it should be easy enough to refactor if needed.

@iunanua iunanua changed the title Include the extra services found in spans in the remote config client request payloads Include the extra services found in spans in the remote config client request payloads [APPSEC-9476] Sep 28, 2023
@iunanua iunanua merged commit 5af860c into master Sep 28, 2023
109 checks passed
szegedi pushed a commit that referenced this pull request Oct 12, 2023
… request payloads [APPSEC-9476] (#3635)

* Register and send extra services found in spans
@szegedi szegedi mentioned this pull request Oct 12, 2023
szegedi pushed a commit that referenced this pull request Oct 12, 2023
… request payloads [APPSEC-9476] (#3635)

* Register and send extra services found in spans
@szegedi szegedi mentioned this pull request Oct 12, 2023
szegedi pushed a commit that referenced this pull request Oct 17, 2023
… request payloads [APPSEC-9476] (#3635)

* Register and send extra services found in spans
szegedi pushed a commit that referenced this pull request Oct 17, 2023
… request payloads [APPSEC-9476] (#3635)

* Register and send extra services found in spans
@iunanua iunanua deleted the igor/rc-extra-services branch December 19, 2023 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants