feat(llmobs): add experiments dataset operations - #9458
Conversation
Overall package sizeSelf size: 7.52 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.2 | 124.41 kB | 440.65 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🔄 Datadog retried 2 tests - 2 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 947c4bb | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9458 +/- ##
==========================================
- Coverage 98.45% 98.40% -0.06%
==========================================
Files 947 952 +5
Lines 128063 130038 +1975
Branches 10779 10998 +219
==========================================
+ Hits 126087 127959 +1872
- Misses 1976 2079 +103
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-07-28 19:41:15 Comparing candidate commit 947c4bb in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2318 metrics, 40 unstable metrics.
|
76fe6a2 to
a218e70
Compare
40c29bd to
f6ffbcd
Compare
![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: e3f8e89d77
ℹ️ 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".
| for (let i = 0; i < header.length; i++) row[header[i]] = values[i] ?? '' | ||
| return { | ||
| inputData: selectedColumns(row, inputDataColumns), | ||
| expectedOutput: selectedColumns(row, expectedOutputColumns), |
There was a problem hiding this comment.
When expectedOutputColumns is omitted or explicitly empty, this still sets every CSV record's expectedOutput to {}. DatasetRecord treats that as a real value, so ensureCreatedAndPushed() emits expected_output: {} and evaluators receive {} instead of the same null/absent expected output that createDataset(..., { records }) uses when expectedOutput is optional. This affects input-only CSV datasets and makes them appear to have an expected output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is an issue if true
| this.#version = response?.data?.attributes?.current_version ?? this.#version | ||
| this.#latestVersion = response?.data?.attributes?.current_version ?? this.#latestVersion |
There was a problem hiding this comment.
For a new dataset with pending records, this stores current_version from the dataset-create response before the later records POST, and nothing below refreshes it after appending. When the create response includes the initial dataset version, Experiment.run() now sends dataset_version from this stale value, so create-and-run flows can pin the experiment to the pre-append/empty dataset version instead of the version containing the records just pushed.
Useful? React with 👍 / 👎.
| records (): Array<{ input: any, expectedOutput: any, metadata: Record<string, any> }> | ||
| version (): number | null | ||
| latestVersion (): number | null | ||
| records (): Array<{ id: string | null, input: any, expectedOutput: any, metadata: Record<string, any> }> |
There was a problem hiding this comment.
This new public contract says records returned from records() have an id, but the pullDataset() path still builds each DatasetRecord without passing the backend item.id and only saves those ids in the separate private recordIds() array. For pulled datasets, pulled.records()[i].id is therefore always null even when the API returned a record id, so callers cannot use the advertised records API to correlate or update pulled rows.
Useful? React with 👍 / 👎.
| const rows = parseCsv(content, { | ||
| delimiter: csvDelimiter, | ||
| bom: true, | ||
| relaxColumnCount: true, | ||
| }) |
There was a problem hiding this comment.
Because the parser options do not skip empty lines, a CSV that contains a blank line between rows or at the end is turned into an extra dataset record with empty strings for the selected columns. In the createDatasetFromCsv() flow this silently uploads and evaluates a bogus empty-input row instead of ignoring the blank line, which is a common shape for hand-edited CSV files.
Useful? React with 👍 / 👎.
| const query = cursor ? `?page[cursor]=${encodeURIComponent(cursor)}` : '' | ||
| const query = new URLSearchParams() | ||
| if (cursor) query.set('page[cursor]', cursor) | ||
| if (version !== undefined && version !== null) query.set('filter[version]', String(version)) |
There was a problem hiding this comment.
When callers pull the latest dataset without an explicit version, the code records current_version from the dataset listing but then fetches records without filter[version], so the records endpoint can return whatever version is current at request time. If the dataset is updated after the listing or between paginated requests, the local rows can come from a newer/mixed version while dataset.version() remains the older value, and Experiment.run() will create the experiment pinned to the wrong dataset version.
Useful? React with 👍 / 👎.
| for (let i = 0; i < header.length; i++) row[header[i]] = values[i] ?? '' | ||
| return { | ||
| inputData: selectedColumns(row, inputDataColumns), | ||
| expectedOutput: selectedColumns(row, expectedOutputColumns), |
There was a problem hiding this comment.
this is an issue if true
sabrenner
left a comment
sabrenner
left a comment
There was a problem hiding this comment.
as discussed offline, csv parsing will be moved to a different PR, so i reviewed the surrounding code. some suggestions but nothing blocking. will approve once the comments i left are either deemed new code worthy or not (although i think maybe only like one or two of them are actually potentially addressable, most are notes for follow-ups)!
| records (): Array<{ input: any, expectedOutput: any, metadata: Record<string, any> }> | ||
| version (): number | null | ||
| latestVersion (): number | null | ||
| records (): Array<{ id: string | null, input: any, expectedOutput: any, metadata: Record<string, any> }> |
There was a problem hiding this comment.
i would say in a follow up if we can more strongly type this expectedOutput and input other than any, that'd be great (i could just be missing some context though, but as a rule of thumb it's a good practice to avoid any typing where possible!)
There was a problem hiding this comment.
ahh ok thanks for pointing out; i'll mimic what we have for python here
| fs.writeFileSync(csvPath, 'id;question;context;answer\nr1;"hello;world";ctx;ok\nr2;bye;ctx2;no\n') | ||
|
|
||
| const calls = [] | ||
| global.fetch.callsFake(async (url, opts) => { |
There was a problem hiding this comment.
nit for a follow-up: generally, i like doing anything like global overrides, or any kind of mocking, in beforeEach hooks and restore in afterEach hooks. otherwise, if this test fails, this mock could pollute other tests and have some unwanted side effects
| assert.equal(create.body.data.attributes.ensure_unique, true) | ||
| assert.deepEqual(create.body.data.attributes.config, { approach: 'kw' }) | ||
| }) | ||
|
|
||
| it('uses the version returned by appending records when creating an experiment', async () => { | ||
| installFetch(calls, { |
There was a problem hiding this comment.
another note for follow ups - we can probably use the VCR for these tests like we do in Python so we can make real calls to the actual backend and just record responses. could be nicer than mocking out, but def not necessary for this PR
| createDataset (name, descriptionOrOptions) { | ||
| return this.experiments.createDataset(name, descriptionOrOptions) | ||
| } | ||
|
|
||
| createDatasetFromCsv (csvPath, name, options) { | ||
| return this.experiments.createDatasetFromCsv(csvPath, name, options) | ||
| } | ||
|
|
||
| pullDataset (name, options) { | ||
| return this.experiments.pullDataset(name, options) | ||
| } | ||
|
|
There was a problem hiding this comment.
just wanna check on the semantics here - to me, it feels like it would make more sense to keep these under experiments, ie
llmobs.experiments.createDataset(...)etc., and not expose them at the top level (they just call the underlying experiments object anyways). just my two cents tho, not blocking.
There was a problem hiding this comment.
yeah you're right, i'll reduce the scope so its just exposed via the experiments object
| return new NoopExperiments('LLM Observability is not enabled') | ||
| } | ||
|
|
||
| createDataset (name, descriptionOrOptions) { |
There was a problem hiding this comment.
if we do decide to remove these from the top-level interface, then we can remove these as well, as a note!
There was a problem hiding this comment.
note for a follow-up - throwing here seems aggressive if it's a no-op because someone intentionally disabled experiments. a warning log or something might be nicer. but i could also be missing context on the experience we wanna offer for users, so feel free to ignore or correct me!
There was a problem hiding this comment.
totally makes sense! will change to warn log to be more graceful
Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
…obs-node-experiments-p0
sabrenner
left a comment
sabrenner
left a comment
There was a problem hiding this comment.
mostly optional nits again except for testing - it's alright, i think the old style of testing is preferable to a force-created cassette (if you look at the bodies and file titles of some other cassettes, example, you'll see that they have specific signatures and file contents that were auto-generated by the VCR capabilities of the testagent). i ideally don't wanna mix hand-made/force-created cassettes in with the generated ones from the testagent.
seeing as that is more work than it might be worth for just landing this, i think reverting back to the old testing style with inline mocks is OK! we can do the VCR approach in a follow-up down the line, if that sounds good 😄
| const queryString = query.toString() ? `?${query.toString()}` : '' | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const resp = await this.#client.request( | ||
| 'GET', | ||
| `${API_BASE_PATH}/${projectId}/datasets/${datasetId}/records${query}` | ||
| `${API_BASE_PATH}/${projectId}/datasets/${datasetId}/records${queryString}` | ||
| ) |
There was a problem hiding this comment.
| const queryString = query.toString() ? `?${query.toString()}` : '' | |
| // eslint-disable-next-line no-await-in-loop | |
| const resp = await this.#client.request( | |
| 'GET', | |
| `${API_BASE_PATH}/${projectId}/datasets/${datasetId}/records${query}` | |
| `${API_BASE_PATH}/${projectId}/datasets/${datasetId}/records${queryString}` | |
| ) | |
| // eslint-disable-next-line no-await-in-loop | |
| const resp = await this.#client.request( | |
| 'GET', | |
| `${API_BASE_PATH}/${projectId}/datasets/${datasetId}/records?${query.toString()}` | |
| ) |
try this out but i think this should be fine? this way we dont have to stringify the query params twice. if it's an empty string, we'll just have something like /proj_id/datasets/dataset_id/record?, which should still be a valid url
There was a problem hiding this comment.
yeah looks cleaner and seems like it will work, will double check
There was a problem hiding this comment.
i don't think these are cassettes created from using the VCR in the testagent docker fixture defined in docker-compose.yml. to use it, you can
- in
docker-compose.yml, add an entry underVCR_PROVIDER_MAPforexperiments, ie
VCR_PROVIDER_MAP=claude-agent-sdk=https://api.anthropic.com,experiments=https://api.datadoghq.com/api/v2/llm-obs/v1/proj/datasets`- start up the container
docker compose up -d testagent- locally, run tests with a
DD_API_KEYto record real responses one time that can be re-used in CI without any additional configuration (hopefully!).
but it might be more work than it's worth. i think how you had the tests before with inline mocks was OK, and i think between that and this (ie force-created cassettes), reverting back to the other testing style of mocking is preferable.
Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
|
Addressed the experiments cassette feedback: removed the hand-made mock cassette files and moved those tests back to inline/stubbed client responses. Will follow up with correctly constructed cassettes |
* feat(llmobs): add experiments dataset operations * feat(llmobs): add CSV dataset creation * chore(llmobs): use csv parser for dataset import * chore(llmobs): add csv parser license * feat(llmobs): include dataset version in experiments * fix(llmobs): address dataset review feedback * create jsontype to replace any * remove dataset operations from llmobs * remove csv dataset func * noops produce warn logs instead of error throwing * use vcr testing instead * Update packages/dd-trace/src/llmobs/experiments/index.js Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): address experiments ci failures * test(llmobs): cover experiments noop accessors * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): simplify dataset records query route * test(llmobs): remove handmade experiment cassettes * test(llmobs): isolate experiment fetch mocks --------- Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
* feat(llmobs): add experiments dataset operations * feat(llmobs): add CSV dataset creation * chore(llmobs): use csv parser for dataset import * chore(llmobs): add csv parser license * feat(llmobs): include dataset version in experiments * fix(llmobs): address dataset review feedback * create jsontype to replace any * remove dataset operations from llmobs * remove csv dataset func * noops produce warn logs instead of error throwing * use vcr testing instead * Update packages/dd-trace/src/llmobs/experiments/index.js Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): address experiments ci failures * test(llmobs): cover experiments noop accessors * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): simplify dataset records query route * test(llmobs): remove handmade experiment cassettes * test(llmobs): isolate experiment fetch mocks --------- Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
* feat(llmobs): add experiments dataset operations * feat(llmobs): add CSV dataset creation * chore(llmobs): use csv parser for dataset import * chore(llmobs): add csv parser license * feat(llmobs): include dataset version in experiments * fix(llmobs): address dataset review feedback * create jsontype to replace any * remove dataset operations from llmobs * remove csv dataset func * noops produce warn logs instead of error throwing * use vcr testing instead * Update packages/dd-trace/src/llmobs/experiments/index.js Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): address experiments ci failures * test(llmobs): cover experiments noop accessors * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): simplify dataset records query route * test(llmobs): remove handmade experiment cassettes * test(llmobs): isolate experiment fetch mocks --------- Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
* feat(llmobs): add experiments dataset operations * feat(llmobs): add CSV dataset creation * chore(llmobs): use csv parser for dataset import * chore(llmobs): add csv parser license * feat(llmobs): include dataset version in experiments * fix(llmobs): address dataset review feedback * create jsontype to replace any * remove dataset operations from llmobs * remove csv dataset func * noops produce warn logs instead of error throwing * use vcr testing instead * Update packages/dd-trace/src/llmobs/experiments/index.js Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): address experiments ci failures * test(llmobs): cover experiments noop accessors * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * Apply suggestion from @sabrenner Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com> * fix(llmobs): simplify dataset records query route * test(llmobs): remove handmade experiment cassettes * test(llmobs): isolate experiment fetch mocks --------- Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
3 participants
What does this PR do?
Adds the Node.js P0 dataset operations surface for LLMObs Experiments, aligned with the Python SDK flow where datasets are created locally, pushed to Datadog, and then pulled back for validation/reuse.
Dataset creation and push/pull
tracer.llmobs.createDataset(name, options)as a top-level alias fortracer.llmobs.experiments.createDataset(...).createDataset(name, { description, records }).await dataset.push()to create the remote dataset and upload records.await tracer.llmobs.pullDataset(name, options)as a top-level alias forexperiments.pullDataset(...).pullDataset(name, { expectedRecordCount, maxWaitMs }).Dataset versions
pullDataset(name, { version }).dataset.version()anddataset.latestVersion().dataset_versionin experiment create payloads when a pulled/pinned dataset version is available, so experiment runs are tied to the dataset version they used.CSV dataset creation
tracer.llmobs.createDatasetFromCsv(csvPath, name, options).inputDataColumnsexpectedOutputColumnsmetadataColumnsidColumncsvDelimiter.csv-parserather than a hand-written CSV parser.Record IDs and validation
createDataset(..., { records })and CSV import.Types, URLs, and tests
index.d.tsandindex.d.v5.ts.datad0g.comtodd.datad0g.com.Motivation
This is the dataset-focused slice of the Node.js LLMObs Experiments P0 SDK work. It brings the Node dataset API closer to the existing
dd-trace-pyexperiments ergonomics while keeping deeper dataset parity, such as tags, update/delete, batch update, and bulk upload, for follow-up work.Testing notebooks / examples
Validation
Validated locally with targeted LLMObs experiments tests and lint.
./node_modules/.bin/eslint packages/dd-trace/src/llmobs/experiments/index.js packages/dd-trace/src/llmobs/experiments/dataset.js packages/dd-trace/src/llmobs/experiments/noop.js packages/dd-trace/src/llmobs/sdk.js packages/dd-trace/src/llmobs/noop.js packages/dd-trace/test/llmobs/experiments/index.spec.js unset OTEL_TRACES_EXPORTER OTEL_LOGS_EXPORTER OTEL_METRICS_EXPORTER ./node_modules/.bin/mocha packages/dd-trace/test/llmobs/experiments/index.spec.js ./node_modules/.bin/mocha packages/dd-trace/test/llmobs/experiments/experiment.spec.jsFollow-up work not included here