Repository navigation
Flush LLM Obs intake writer synchronously in serverless environments - #12249
purple4reina wants to merge 12 commits into
Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4196c2186d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
When LLM Observability and AppSec API Security run together outside serverless mode, both writer threads can post-process the same trace. This can close one AppSec request context twice and release its limit permit twice.
🤖 Datadog Autotest · Commit 4196c21 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
purple4reina
changed the title
|
Layer published from current commit (b83d309) E2e pipeline running this layer (executing just java on lambda-features suite) https://gitlab.ddbuild.io/DataDog/serverless-e2e-tests/-/pipelines/133218539 |
|
Pipeline passed. Java numbers are all at 5's now, showing that this PR fixes the underlying issue. 
|
purple4reina
added
type: bug fix
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b83d309c8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b83d309 to
ced2ef1
Compare
dd-octo-sts
Bot
added
the
tag: ai generated
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49858a3b59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
Since this change effectively only applies in serverless, I think this change is fine from performance perspective.
There was a problem hiding this comment.
@dougqh Indeed automatic flushing (.alwaysFlush(isServerlessDefault)) after each write is serverless-only, but I believe the change to TraceProcessingWorker method affect all callers of flush. which makes me careful on this change.
After some code search, it seems it affects in particular, and writer shutdown and CI Visibility. Which makes these callers now wait for the secondary queue as well. That it might be a good thing to properly handle this.
Since it affects more usecases, I believe my concern on the timeout needs to be addressed.
@purple4reina In my comment I give some code example which I believe to be fixing the issue with deadline. Note I also suggest a test that exercises this scenario.
The DDAgentWriter branch of WriterFactory never wired up an LLM Obs DDIntakeWriter, so LLM Obs spans were silently dropped whenever the tracer picked DDAgentWriter (e.g. in Lambda/serverless mode with CI Visibility disabled). Broadcast to a dedicated LLM Obs DDIntakeWriter via MultiWriter, flushed synchronously in serverless environments just like the primary writer.
The DDAgentWriter branch's dedicated LLM Obs DDIntakeWriter duplicated the LLM Obs track that the DD_INTAKE_WRITER_TYPE branch already builds whenever llmObsEnabled is true (Agent.java defaults to MultiWriter:DDIntakeWriter,DDAgentWriter for that case), so every span was sent twice. Verified live on Lambda: the pre-fix build sent LLMOBS/v2 payloads twice per invocation. The original bug this branch was fixing -- spans silently dropped in Lambda -- was actually caused by the DDIntakeWriter branch never setting alwaysFlush, so the periodic flush timer rarely beat the execution environment freezing after the handler returns. Verified live: without alwaysFlush, 0/8 invocations delivered a span; with it, 8/8 did, one send each.
Sampled-out traces route to the secondary queue, which flush() never touched, so a Lambda invocation whose execution environment freezes right after the synchronous flush returns can lose them. Verified live: 1/15 delivered without this fix, 15/15 with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both the DDIntakeWriter and DDAgentWriter branches computed the same config.isAgentConfiguredUsingDefault() && isRunningInServerlessEnvironment() condition separately; hoist it into a single isServerlessDefault local. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 38a42d6: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 59fb1c5:
What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 347fd35: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 031fb73: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 35c776b: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
This PR is rejected because it was updated |
The merge queue reported infra failures (git checkout timeouts) for some attempts, but every attempt that got far enough also failed DemoExecutorServiceTest on IBM JDK 8 (7 of 7 runs since Oct 2; the test passes on ibm8 elsewhere). The queue cancels the pipeline at the first failed job, so the bot comment does not always show this failure. Cause: on IBM JDK 8 the agent delays starting the writer by at least 100ms (okHttpDelayMillis, because of IBMSASL). The ExecutorService app exits before that, so the shutdown flush runs before the serializer thread starts. This branch checked serializerThread.isAlive() before the first offer, so flush() returned false at once, the writer closed, and the trace was never sent. Before this branch, flush() enqueued the FlushEvent and waited, and the late-starting serializer drained it. Fix: always offer once, and check the thread only while the queue is full. The new test fails without this change (expected true, was false) on any JVM. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
4 participants
Context
dd-trace-java's LLM Obs writer doesn't respect the S in CBAS(H) (note that the H is silent). The aws lambda runtime gets suspended between invocations, yet LLM Obs data was being sent on a periodic schedule. This meant if the function was not actively running when the flush struck, data wouldn't be sent.
Summary
The
DD_INTAKE_WRITER_TYPEbranch ofWriterFactory.createWriter()— which is what actually carries LLM Obs spans, sinceAgent.javadefaults the writer type toMultiWriter:DDIntakeWriter,DDAgentWriterwhenever LLM Obs is enabled — never setalwaysFlush. In Lambda, the execution environment can freeze as soon as the handler returns, before this writer's periodic flush timer next fires, so spans were silently dropped most of the time.This surfaced as
ml_obs.tracerarely/never being emitted for java Lambda functions inserverless-e2e-tests.What changed
Set
alwaysFlushon theDDIntakeWriterbranch the same way theDDAgentWriterbranch already does (config.isAgentConfiguredUsingDefault() && ServerlessInfo.get().isRunningInServerlessEnvironment()), and drop theDDAgentWriterbranch's separate LLM ObsDDIntakeWriter+MultiWriter— it duplicated the track the intake branch already builds, so every span was being sent twice (visible asml_obs.tracereporting exactly 2x the invocation count).In Lambda,
TraceProcessingWorker.flush()now also waits on the secondary queue, so sampled-out traces (e.g.DD_TRACE_SAMPLE_RATE=0) aren't left behind at freeze. The check runs once, fromServerlessInfo, when the worker is built. Outside Lambda it still enqueues a single marker. The one behavior change there: enqueuing a marker now counts against the flush timeout instead of spinning until the queue has room, so a flush against a full queue returnsfalseat the timeout.Validation
Built and deployed custom Lambda layers to a sandbox stack to isolate each half of this fix:
alwaysFlushon the intake branch): 0/8 invocations delivered an LLM Obs span.alwaysFlushchange: 8/8 invocations delivered exactly one span each, confirmed via CloudWatch logs (Successfully sent 1 traces ... LLMOBS/v2).MultiWriterfix that it sent every span twice (two independentLLMOBS/v2payloads per invocation).