-
Notifications
You must be signed in to change notification settings - Fork 362
feat(llmobs): implement multi-tenant routing context support #7158
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(llmobs): implement multi-tenant routing context support #7158
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7158 +/- ##
==========================================
- Coverage 86.25% 86.12% -0.14%
==========================================
Files 513 513
Lines 22054 22106 +52
==========================================
+ Hits 19023 19039 +16
- Misses 3031 3067 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@codex review |
Overall package sizeSelf size: 4.41 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 |
|
To use Codex here, create a Codex account and connect to github. |
|
BenchmarksBenchmark execution time: 2026-01-20 15:52:12 Comparing candidate commit a3d89ff in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 233 metrics, 27 unstable metrics. |
b34e1f5 to
e531090
Compare
| this._eventType = eventType | ||
|
|
||
| this._buffer = [] | ||
| this._buffers = new Map() |
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.
Please use private properties instead of underscores for all changed properties and new ones. Changing old ones is nice to have as separate commit :)
Please also add a comment to our Claude.md file that highlights this. Otherwise new code changes will likely come up by the AI that use the underscore instead.
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.
Done. Converted to # private syntax where possible (#buffers, #getMaskedRoutingKey, #cleanupEmptyBuffers, #getUrlForRouting)
Kept _getRoutingKey and _getOrCreateBuffer with underscore because LLMObsSpanWriter (subclass) needs to access them.
Also added the convention to AGENTS.md (symlink to claude.md ?) under "Class Properties and Methods".
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.
FYI, I did keep things like get _buffer, set _buffer with the underscore because they are backward compatibility shims. You were not thinking about changing them as well, right?
sabrenner
left a comment
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.
mostly small comments but one larger implementation comment i'm curious on your thoughts on. but overall the logic makes sense to me and the sdk api change looks good pending some type definition updates!
sabrenner
left a comment
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.
left a question on the test file added!
sabrenner
left a comment
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.
looks good to me now! just nits, plus some of the nested routing context tests are failing, i'm happy to help with looking into those, so lmk
sabrenner
left a comment
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.
this looks great now! just one small comment (barring no changes for supporting evals in this as well, see below, i promise it's the last one 😭) about the function name we should consider before shipping, but then i'll approve.
i followed up offline, but if we're not supporting evals in this pr we should call that out for those looking over release notes
…LMObs class, remove legacy routing-context module, and enhance multi-tenant event handling with isolated buffers
… and update tests to reflect changes
Co-authored-by: Ruben Bridgewater <[email protected]>
…ions and context handling
…and update buffer management in BaseLLMObsWriter
…de to improve user awareness
3051004 to
a9c0186
Compare
sabrenner
left a comment
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.
really awesome job!! 🚢
* feat(routing): implement multi-tenant routing context support * Removed default parameters from getCurrentRouting, now returns null if no routing context is available. * restore comment + use size AFTER truncation to make flushing decision * refactor(routing-context): simplify getCurrentRouting function and enhance withRoutingContext documentation; add test for empty ddApiKey * Backward compat comment + Nested context test * fix: API Key Logged in Debug Output * chore: remove unnecessary comments * Converted to private # syntax, revised tests, and added rule to AGENTS.md * fix * #originalEndpoint * remove some redundancies * add comment for legacy * refactor(routing-context): implement withRoutingContext directly in LLMObs class, remove legacy routing-context module, and enhance multi-tenant event handling with isolated buffers * refactor * refactor * warn + private methods * refactor(llmobs): simplify warning message for nested routing context and update tests to reflect changes * Update AGENTS.md Co-authored-by: Ruben Bridgewater <[email protected]> * test(llmobs): enhance multi-tenant routing tests with improved assertions and context handling * refactor(llmobs): introduce LLMObsBuffer for improved event handling and update buffer management in BaseLLMObsWriter * trim redundancy * avoid double /evp_proxy/v2/... prefix * feat(llmobs): add warning for routing context usage in agent proxy mode to improve user awareness * lint * Sam's code enhancements * rename withRoutingContext to routingContextt * add support for submitEvaluation * handle signature change --------- Co-authored-by: Ruben Bridgewater <[email protected]>
What does this PR do?
Adds multi-tenant API key support for LLM Observability, enabling platforms to route LLMObs telemetry to different Datadog organizations from a single process.
New API:
Motivation
Multi-tenant AI platforms need to send LLM Observability data to their customers' Datadog organizations. This enables:
Implementation
Key Changes:
withRoutingContext(options, fn)method added to LLMObs SDK (usesAsyncLocalStorage)BaseLLMObsWriterrefactored to multi-buffer architecture (one buffer per API key)Testing
writers/multi-tenant.spec.js- Tests for buffer routing, endpoint construction, API key isolation, context nesting, and concurrency