Skip to content

fix(llmobs): prevent config origin overwrite in enable()#7183

Merged
watson merged 1 commit intomasterfrom
watson/clean-up-llmobs-enable
Jan 8, 2026
Merged

fix(llmobs): prevent config origin overwrite in enable()#7183
watson merged 1 commit intomasterfrom
watson/clean-up-llmobs-enable

Conversation

@watson
Copy link
Copy Markdown
Collaborator

@watson watson commented Jan 6, 2026

What does this PR do?

Fixes a bug in the LLMObs SDK's enable() method where configuration origins were being incorrectly tracked for telemetry:

  • Pass only the llmobs config object to configure() to avoid spreading the entire config
  • Remove unnecessary intermediate variables in both config.js and sdk.js
  • Invert early-return logic in sdk.js for clearer control flow
  • Add TODO comment noting that calling configure() from enable() still sets origin as 'code' even when enable() is called from Remote Config updates

Motivation

The enable() method in packages/dd-trace/src/llmobs/sdk.js was spreading the entire _config object when calling configure():

this._config.configure({ ...this._config, llmobs: llmobsConfig })

This caused all configuration values to be re-set with origin 'code', overwriting their actual origins (e.g. 'env', 'default', etc.). This breaks telemetry tracking which relies on accurate origin information to understand how users are configuring the tracer.

Additional Notes

I've done my best to ensure that it's ok to call configure with only a sub-set of config options and that it will still work even if #applyOptions was called previously with another sub-set of options. But this is important to review!

There's still a remaining issue (noted in the TODO): when enable() is called based on APM_TRACING RC product updates, the config telemetry will show origin as 'code' rather than the actual source (whatever that should actually be?).

The enable() method was spreading the entire config object when calling
configure(), which incorrectly reset all config values with origin 'code',
overwriting their actual origins (e.g. 'env', 'default'). This affects
telemetry tracking of config origins.

- Pass only llmobs config to configure() to avoid overwriting origins
- Remove unnecessary intermediate variables
- Invert early-return logic for clearer control flow
- Add TODO comment about config telemetry origin tracking
@watson watson self-assigned this Jan 6, 2026
Copy link
Copy Markdown
Collaborator Author

watson commented Jan 6, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 6, 2026

Overall package size

Self size: 4.39 MB
Deduped: 5.21 MB
No deduping: 5.21 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.0 | 68.46 kB | 797.03 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |

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

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Jan 6, 2026

Benchmarks

Benchmark execution time: 2026-01-06 21:09:53

Comparing candidate commit 8baa329 in PR branch watson/clean-up-llmobs-enable with baseline commit 2681a5e in branch master.

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

@watson watson marked this pull request as ready for review January 6, 2026 21:10
@watson watson requested review from a team as code owners January 6, 2026 21:10
@watson watson requested review from BridgeAR and removed request for a team January 6, 2026 21:10
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.52%. Comparing base (2681a5e) to head (8baa329).
⚠️ Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
packages/dd-trace/src/llmobs/sdk.js 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7183      +/-   ##
==========================================
+ Coverage   84.51%   84.52%   +0.01%     
==========================================
  Files         525      525              
  Lines       22492    22488       -4     
==========================================
- Hits        19008    19007       -1     
+ Misses       3484     3481       -3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Collaborator

@sabrenner sabrenner left a comment

Choose a reason for hiding this comment

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

thanks for catching this! tbh the code in sdk for this enablement is from a misguided early on approach of mirroring our Python implementation, which does need an enable method to call as it is its own module, but totally not needed here, and in our docs we only ever show enablement through init.

The idea is in v6 we'll remove these methods (reminds me that I need to add a deprecation log), so hopefully this just won't be an issue at that point, but again thank you for catching this in the meantime!

also thanks for the minor cleanups, definitely makes sense 😄

Copy link
Copy Markdown
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

While this already improves things very slightly, it does not seem to fix the overall issue that config things should not be handled outside of config.

Comment thread packages/dd-trace/src/llmobs/sdk.js
Comment thread packages/dd-trace/src/llmobs/sdk.js
@watson watson merged commit 7ae9abd into master Jan 8, 2026
797 of 798 checks passed
@watson watson deleted the watson/clean-up-llmobs-enable branch January 8, 2026 09:19
@dd-octo-sts dd-octo-sts Bot mentioned this pull request Jan 12, 2026
dd-octo-sts Bot pushed a commit that referenced this pull request Jan 12, 2026
The enable() method was spreading the entire config object when calling
configure(), which incorrectly reset all config values with origin 'code',
overwriting their actual origins (e.g. 'env', 'default'). This affects
telemetry tracking of config origins.

- Pass only llmobs config to configure() to avoid overwriting origins
- Remove unnecessary intermediate variables
- Invert early-return logic for clearer control flow
- Add TODO comment about config telemetry origin tracking
nina9753 pushed a commit that referenced this pull request Jan 15, 2026
The enable() method was spreading the entire config object when calling
configure(), which incorrectly reset all config values with origin 'code',
overwriting their actual origins (e.g. 'env', 'default'). This affects
telemetry tracking of config origins.

- Pass only llmobs config to configure() to avoid overwriting origins
- Remove unnecessary intermediate variables
- Invert early-return logic for clearer control flow
- Add TODO comment about config telemetry origin tracking
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.

3 participants