-
Notifications
You must be signed in to change notification settings - Fork 420
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
feat(config): support libdatadog's library_config for config through file #11839
base: main
Are you sure you want to change the base?
Conversation
|
606b2a5
to
cef695a
Compare
cef695a
to
3f85c2a
Compare
3f85c2a
to
908ace8
Compare
BenchmarksBenchmark execution time: 2025-01-24 16:23:27 Comparing candidate commit 1a40d17 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 392 metrics, 2 unstable metrics. scenario:iast_aspects-ospathbasename_aspect
scenario:iast_aspects-ospathsplit_aspect
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 I'd like to suggest not adding new modules under /profiling as the functionality is not profiling. You should be able to expose the libdatadog API through src/core, or add a new rust/python module under src/
4bd0ef9
to
217fa81
Compare
217fa81
to
39af866
Compare
55ce7fe
to
df2f272
Compare
df2f272
to
a1877e2
Compare
Datadog ReportBranch report: ✅ 0 Failed, 208 Passed, 1390 Skipped, 5m 3.12s Total duration (35m 0.17s time saved) |
bb2848b
to
32de0db
Compare
fddf037
to
bac1549
Compare
f54495b
to
b42da4d
Compare
Adding @brettlangdon to the reviewers early so you could get feedback sooner than later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we break up this PR?
I think there is the libdatadog version bump, core package renaming, and then the config file changes which can all get broken up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great! High level comments:
- Setting environment variables directly is good enough for this POC now but we will need to revisit this design decision to better track the origin of configurations.
- We need to identify all scenarios where the value set by stable configurations can be overriden by the ddtrace library.
- We need to document a clear precedence for configurations and ensure all libraries follow it (rc -> stable_config -> in_code -> envar -> reading from yaml file)
- We need to document which configurations are supported by
15.0
. Including this in the PR description is fine. - We should split up this PR (@brettlangdon's comment)
ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker.cpp
Outdated
Show resolved
Hide resolved
ddtrace/internal/datadog/profiling/dd_wrapper/src/receiver_interface.cpp
Outdated
Show resolved
Hide resolved
releasenotes/notes/libdatadog-library_config-3c36c4f868d2c86f.yaml
Outdated
Show resolved
Hide resolved
- nodejs | ||
operator: equals | ||
configuration: | ||
DD_SERVICE: my-service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the python tracer the DD_SERVICE environment variable can be overridden by integrations and other ddtrace products (ex: serverless).
We will need to audit all the configurations supported by libdatadog and ensure the value set via APM Stable Configurations take precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I also think that we need to have a discussion on the order of precedence. For instance in the agent, config values set by the runtime (ie inside the agent code) take precedence over Fleet Automation. One could argue that we should replicate that behaviour here; but imo this is a product decision.
d1972ff
to
e207136
Compare
…brary-config-libdatadog
e207136
to
be6fc28
Compare
@brettlangdon @mabdinur Could you please review #12063 & #12065 as new prerequisites to this PR, as suggested? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Once the prerequisite PRs are merged we can ship this change
This PR depends on #12063. Rename src/core to src/native. This is laying ground as a [following PR](#11839) will need to import native code in `ddtrace.__init__.py`. Without this change this would create a dependency loop between `ddtrace` & `ddtrace.core` through `event_hub.py`. This PR only renames files, so a non-regression test is the only thing needed. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
Description of changes
libdatadog
to v15.0.0 & handles the required changes to crashtrackinglibdatadog
that allows reading configuration from a file with a higher precedence than environment variablesSupported config entries
As of libdatadog v15.0.0 (source):
DD_TRACE_DEBUG
DD_SERVICE
DD_ENV
DD_VERSION
DD_PROFILING_ENABLED
Testing strategy
Added a unit test.
To test this PR, you can:
/etc/datadog-agent/managed/datadog-apm-libraries/stable/libraries_config.yaml
Additional concerns
This change does increase the size of the serverless layer by 380KB (14204KB vs 13824 as the limit) even after optimization. I had to go from
lto=true
tolto=fast
, aslto=true
somehow prevents release builds on older machines. That accounts for 200KB, while the 200 other KB are mostly coming from the libdatadog bump.Checklist
Reviewer Checklist