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

Active tracing configuration #3928

Merged
merged 151 commits into from
Feb 27, 2024
Merged

Conversation

ida613
Copy link
Collaborator

@ida613 ida613 commented Jan 4, 2024

What does this PR do?

Adds origin field in telemetry event app-started configuration

Architecture

Each configuration value is set in the following priority: options user passes to the Config constructor, environment variable, then default. Telemetry app-started requires the tracer to send configuration values along with which of the three ways each value is set. So for the config values telemetry wants to report, instead of calling coalesce, we first call _applyDefaults, which sets all of them to their default values. Then we call _applyEnvironment, which sets the config values to their corresponding environment variables if they are available. Lastly we call _setOptions which applies the available options. The function _merge finds out which configuration values have been changed and which container they are in. This way, we can send the configuration value changes to telemetry, along with their origin.

Since this PR changes how a lot of config values are set, we are requesting reviews from ASM, profiling and ci-visibility to make sure everything works as it should.

Additional Notes

Security

Datadog employees:

  • 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.

Unsure? Have a question? Request a review!

Copy link

github-actions bot commented Jan 4, 2024

Overall package size

Self size: 6.13 MB
Deduped: 62.01 MB
No deduping: 62.77 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 1.7.0 16.71 MB 16.72 MB
@datadog/native-appsec 7.0.0 14.51 MB 14.52 MB
@datadog/pprof 5.0.0 9.59 MB 10.44 MB
protobufjs 7.2.5 2.77 MB 6.56 MB
@datadog/native-iast-rewriter 2.2.3 2.19 MB 2.28 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.7.3 67.62 kB 731.01 kB
pprof-format 2.0.7 588.12 kB 588.12 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
shell-quote 1.8.1 44.96 kB 44.96 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
tlhunter-sorted-set 0.1.0 24.94 kB 24.94 kB
limiter 1.1.5 23.17 kB 23.17 kB
dc-polyfill 0.1.4 23.1 kB 23.1 kB
retry 0.13.1 18.85 kB 18.85 kB
node-abort-controller 3.1.1 16.89 kB 16.89 kB
jest-docblock 29.7.0 8.99 kB 12.76 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 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

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: Patch coverage is 99.27273% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.24%. Comparing base (f91457f) to head (fec9045).
Report is 4 commits behind head on master.

Files Patch % Lines
packages/dd-trace/src/config.js 99.61% 1 Missing ⚠️
packages/dd-trace/src/telemetry/index.js 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3928      +/-   ##
==========================================
+ Coverage   84.99%   85.24%   +0.24%     
==========================================
  Files         247      247              
  Lines       10736    10848     +112     
  Branches       33       33              
==========================================
+ Hits         9125     9247     +122     
+ Misses       1611     1601      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Jan 4, 2024

Benchmarks

Benchmark execution time: 2024-02-26 19:25:13

Comparing candidate commit fec9045 in PR branch ida613/active-tracing-configuration with baseline commit f91457f in branch master.

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

Copy link
Member

@simon-id simon-id left a comment

Choose a reason for hiding this comment

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

LGTM for appsec config.
I've noticed some inconsistencies in the config entries types with _set{type}() functions, but nothing that can't be fixed later by our team 👍

Copy link
Contributor

@szegedi szegedi left a comment

Choose a reason for hiding this comment

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

🚢

Copy link
Collaborator

@juan-fernandez juan-fernandez left a comment

Choose a reason for hiding this comment

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

looks good from ci vis' perspective

Copy link
Collaborator

@astuyve astuyve left a comment

Choose a reason for hiding this comment

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

The config refactor looks good to me for Serverless. Let me know when an RC build is available and I can test it out a bit inside Lambda

@ida613 ida613 merged commit 04f1370 into master Feb 27, 2024
109 of 111 checks passed
@ida613 ida613 deleted the ida613/active-tracing-configuration branch February 27, 2024 16:35
@rochdev
Copy link
Member

rochdev commented Feb 27, 2024

Btw, Igor and me thought that it would be a better dev experience to have each config entry be grouped by entry instead of grouped by default/env/programmatic. I know it's a bit late to change the whole architecture now but something like this would have been so much better:

config.setBool('appsec.enabled', 'default', false)
config.setBool('appsec.enabled', 'env', process.env.DD_APPSEC_ENABLED)
config.setBool('appsec.enabled', 'prog', config.appsec.enabled)

// repeat blocks like this for each entry...

We could do that, it would just result in a giant function. I've ordered the config values if that helps. I do see the advantage of your proposal. @rochdev what do you think?

or some kind of builder could work?

config.bool('appsec.enabled')
  .default(false)
  .env(process.env.DD_APPSEC_ENABLED)
  .prog(config.appsec.enabled)  // prog or prop?
  .set()

or

config.setBool(Property.new('appsec.enabled')
  .default(false)
  .env(process.env.DD_APPSEC_ENABLED)
  .prog(config.appsec.enabled))

All the property config would be in the same place, it is readable, and quite flexible but idk if it would meet the requirements.

I know the PR is merged, but just for clarification, config is split by type instead of by option by design in the new code. This is to make sure that all the options are set for one type before moving on to the other type so that everything happens and is tracked in stages, and is applied in the correct order. Doing every option one by one with some sort of fluent interface would definitely be cleaner, but it wouldn't achieve the intended goal of this PR.

uurien pushed a commit that referenced this pull request Mar 4, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
uurien pushed a commit that referenced this pull request Mar 5, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
uurien pushed a commit that referenced this pull request Mar 5, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
This was referenced Mar 5, 2024
uurien pushed a commit that referenced this pull request Mar 6, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
uurien pushed a commit that referenced this pull request Mar 6, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
uurien pushed a commit that referenced this pull request Mar 7, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
uurien pushed a commit that referenced this pull request Mar 7, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
uurien pushed a commit that referenced this pull request Mar 7, 2024
Co-authored-by: Thomas Hunter II <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
Co-authored-by: simon-id <[email protected]>
Co-authored-by: Attila Szegedi <[email protected]>
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.