fix(llmobs): submit eval metrics through the shared HTTP clients - #12154
fix(llmobs): submit eval metrics through the shared HTTP clients#12154ddog-thibault-nadin wants to merge 2 commits into
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aef47d25b2
ℹ️ 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".
| + "/" | ||
| + EVAL_METRIC_API_PATH); | ||
| headers = Headers.of(DD_API_KEY_HEADER_NAME, config.getApiKey()); | ||
| httpClient = sco.getIntakeHttpClient(); |
There was a problem hiding this comment.
Keep HTTPS eval submissions off the cleartext intake client
When DD_FORCE_CLEAR_TEXT_HTTP_FOR_INTAKE_CLIENT is enabled, getIntakeHttpClient() builds the shared intake client as cleartext-only via OkHttpUtils.buildHttpClient, but the agentless eval URL above is still hard-coded to https://api.<site>/.... OkHttp rejects HTTPS requests on a ConnectionSpec.CLEARTEXT-only client, so enabling that supported intake flag for another backend intake now disables LLMObs agentless eval submissions; use an HTTPS-capable client for HTTPS eval URLs or honor the configured LLMObs agentless URL before sharing this client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@ddog-thibault-nadin could you look to this comment please
There was a problem hiding this comment.
More details
The worker now receives the already-configured shared client in both agent and agentless modes, preserving UDS/named-pipe routing and intake timeout/clear-text settings. Focused compilation and the shared communication tests found no reproducible behavioral regression; no additional tests recommended because the available production lifecycle initializes the shared clients before the worker.
📊 Validated against 2 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit aef47d2 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
The new agentless URL validation accepts ftp:// and file://, but the worker passes them to OkHttp’s HTTP(S)-only parser during startup. A malformed URL configuration can therefore disable LLM Observability instead of falling back to the default intake.
📊 Validated against 11 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 8dc93b3 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
|
||
| final String llmObsAgentlessBaseUrlStr = configProvider.getString(LLMOBS_AGENTLESS_URL); | ||
| llmObsAgentlessBaseUrl = | ||
| isValidUrl(llmObsAgentlessBaseUrlStr) |
There was a problem hiding this comment.
Reject URL schemes unsupported by OkHttp
A syntactically valid but unsupported configured scheme prevents LLM Observability initialization instead of safely using the default intake.
Assertion details
- Input: Enable agentless LLMObs and set
DD_LLMOBS_AGENTLESS_URLtoftp://localhost:9126orfile:///tmp/llmobs. - Expected:
The shared URL validator should accept only HTTP and HTTPS schemes, causing unsupported schemes to fall back to the default intake. Because sibling CI Visibility call sites use the same helper, a line-local suggestion here would be incomplete. - Actual: The new initialization uses generic
isValidUrl, which acceptsftp://andfile://. Both temporary cases retained the value, althoughEvalProcessingWorkersubsequently passes it to OkHttp's HTTP(S)-onlyHttpUrl.get. The shared validator is also used by sibling CI Visibility intake URL configuration, so the complete fix belongs in that helper rather than only this assignment.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
What Does This Do
Makes
EvalProcessingWorkerpost through the HTTP clients held bySharedCommunicationObjectsinstead of a rawnew OkHttpClient():sco.agentHttpClientsco.getIntakeHttpClient()Motivation
When the Agent is reached over a Unix domain socket or a named pipe,
SharedCommunicationObjects.parseAgentUrlonly stores a placeholder URL:The real transport is the socket factory configured on
agentHttpClient. The worker built its own client, so it posted to that placeholderhost:portwhere nothing listens:SubmitEvaluationsilently reaches nobody in those supported setups.The agentless branch is not broken in the same way, but its own client ignored
getAgentTimeout()andforceClearTextHttpForIntakeClient.getIntakeHttpClient()honours both and is built without UDS/named pipe, which is what a direct intake call needs.LLMObsSystem.startalready callssco.createRemaining(config)before constructing the worker, so both clients are initialised by then.