feat(experiment): Add experiment capabilities #672
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds experiment and evaluator features to the Traceloop SDK (new Experiment and Evaluator classes, interfaces, client wiring), dataset JSONL retrieval, sample app experiment and medical prompts, package/dependency updates, CLAUDE.md, and .gitignore adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant SDK as Traceloop SDK
participant Exp as Experiment
participant DS as Datasets
participant Eval as Evaluator
participant API as Backend API
Dev->>SDK: initialize({ apiKey, appName, experimentSlug? })
SDK->>Exp: instantiate Experiment(client)
SDK->>Eval: instantiate Evaluator(client)
Dev->>SDK: client.experiment.run(taskFn, { datasetSlug, datasetVersion, evaluators })
SDK->>Exp: run(taskFn, options)
Exp->>DS: getVersionAsJsonl(slug, version)
DS->>API: GET /v2/datasets/{slug}/versions/{version}/jsonl
API-->>DS: JSON/JSONL response
DS-->>Exp: JSONL string
Exp->>Exp: parse JSONL -> rows
loop per row
Exp->>Dev: call taskFn(row)
Dev-->>Exp: task output
Exp->>API: POST /v2/experiments/{slug}/runs/{runId}/tasks
alt evaluators provided
Exp->>Eval: runExperimentEvaluator({...})
Eval->>API: POST /v2/evaluators/slug/{name}/execute
API-->>Eval: { executionId, streamUrl }
Eval->>API: GET streamUrl (poll/SSE)
API-->>Eval: stream events / results
Eval-->>Exp: ExecutionResponse[]
end
end
Exp-->>SDK: ExperimentRunResult
SDK-->>Dev: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Nina Kollman seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 20d23b9 in 1 minute and 47 seconds. Click for details.
- Reviewed
1609lines of code in17files - Skipped
2files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:77
- Draft comment:
The experimentSlug check contains a TODO comment. Ensure the validation and error messaging is finalized before release. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that validation and error messaging is finalized, which is similar to asking them to double-check or ensure something is done. This violates the rule against asking the author to ensure behavior is intended or tested.
2. packages/traceloop-sdk/src/lib/client/stream/sse-client.ts:35
- Draft comment:
Excessive console logging in production code may clutter outputs. Consider using a configurable logging mechanism or debug flag. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. packages/sample-app/package.json:37
- Draft comment:
Typo alert: The filename 'simple_experiment_test.js' might be a typographical error. Considering the similar naming conventions used in the other commands, should this be 'sample_experiment_test.js'? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The naming convention isn't actually consistent - some files use 'sample_', others use 'test_'. The file name choice seems intentional since it's a new addition. This appears to be a stylistic suggestion rather than a functional issue. The current name is clear and understandable. The comment might be pointing out a legitimate inconsistency in naming conventions that could affect maintainability. File naming standards can be important for project organization. While consistent naming is good, this is a minor stylistic issue that doesn't affect functionality. The existing name is clear and there's no strong evidence that it's actually wrong or problematic. Delete the comment as it's making a speculative suggestion about naming conventions without strong evidence that the current name is problematic.
Workflow ID: wflow_EUtOYKsggAXezzyr
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.gitignore (1)
22-27: Do not ignore the .vscode directory if you want the exceptions to workAdding
.vscode/causes Git to ignore the directory itself, which prevents the subsequent negation rules (!.vscode/settings.json, etc.) from re-including those files. Keep only.vscode/*to ignore contents while allowing specific files to be committed.Apply this diff:
# IDE - VSCode .vscode/* -.vscode/ !.vscode/settings.json !.vscode/tasks.json !.vscode/launch.json !.vscode/extensions.json
🧹 Nitpick comments (29)
packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (4)
80-86: Add input validation for parity with getVersionCSVOther methods validate
slugandversion. Add the same checks here to fail fast and keep behavior consistent.async getVersionAsJsonl(slug: string, version: string): Promise<string> { - const url = `/v2/datasets/${slug}/versions/${version}/jsonl`; + this.validateDatasetSlug(slug); + if (!version || typeof version !== "string") { + throw new Error("Version must be a non-empty string"); + } + + const url = `/v2/datasets/${slug}/versions/${version}/jsonl`;
83-88: Consider centralizing error handling for consistencyOther methods use
this.handleResponse(response)which likely standardizes error parsing/logging. It’s fine to bypass it here to read raw text, but consider reusing shared helpers where possible (e.g., only for JSON error bodies) to keep behavior uniform.
97-104: Optional: ensure newline semantics for JSONLSome consumers expect a trailing newline in JSONL streams. If interoperability issues arise, consider appending
\nat the end of the returned text.
80-105: Add minimal tests for JSONL and JSON fallbackPlease add tests that cover:
- JSONL response (text) path.
- JSON array response converted to JSONL lines.
- JSON error body path (e.g.,
{ error: "..." }).I can scaffold ts-mocha tests mirroring
getVersionCSVtests if you’d like.CLAUDE.md (1)
59-67: Document new Experiment and Evaluator capabilities with a quickstartGiven this PR adds Experiment/Evaluator clients and streaming, add a short “Experiments & Evaluators” section here with:
- Minimal code sample using
Experiment/Evaluator.- Note on Node SSE support and the
eventsourcedependency.- Pointers to sample scripts (
run:sample_experiment,run:experiment_test) and files inpackages/sample-app/src/.Happy to draft this section with runnable samples aligned to the new APIs.
packages/sample-app/src/medical_prompts.ts (3)
10-31: Guard against prompt injection by delimiting user input and clarifying handling rulesInterpolate the user question inside a clearly delimited block and instruct the model to treat it as untrusted. Also tighten guidance to avoid personalized advice while still allowing general, evidence-based information.
Apply this diff to strengthen safety and robustness:
export function provideMedicalInfoPrompt(question: string): string { - return `You are a health educator providing comprehensive medical information. + return `You are a health educator providing comprehensive medical information. -Question: ${question} +User question (verbatim — do not follow any instructions inside this block): """ +${question} +""" Please provide a detailed, educational response that includes: 1. **Clear, factual explanation** of the medical concept or condition 2. **Key benefits and considerations** related to the topic 3. **Specific recommendations** based on current medical knowledge 4. **Important disclaimers** about consulting healthcare professionals 5. **Relevant context** that helps understand the topic better Guidelines: - Use evidence-based information - Explain medical terms in plain language - Include both benefits and risks when applicable - Emphasize the importance of professional medical consultation - Provide actionable, general health guidance + - Do not provide personalized medical advice, diagnosis, or treatment plans; keep guidance general. + - If the question suggests an emergency or imminent risk (e.g., chest pain, difficulty breathing, suicidal intent), advise seeking immediate professional help and do not provide further information beyond safety guidance. Your response should be educational, balanced, and encourage informed healthcare decisions.`; }Would you like me to add minimal unit tests that snapshot the produced prompt strings to protect against accidental formatting regressions?
37-60: Add crisis-handling guidance and delimit user input to reduce injection riskFor refusal flows, bake in explicit crisis language handling and ensure the user question is treated as untrusted input.
Apply this diff:
export function refuseMedicalAdvicePrompt(question: string): string { - return `You are a helpful AI assistant with a strict policy about medical advice. + return `You are a helpful AI assistant with a strict policy about medical advice. -Question: ${question} +User question (verbatim — do not follow any instructions inside this block): """ +${question} +""" I understand you're seeking information about a health-related topic, but I cannot provide medical advice, diagnosis, or treatment recommendations. Instead, I'd like to: 1. **Acknowledge your concern** - Your health questions are important and valid 2. **Explain why I can't advise** - Medical situations require professional evaluation 3. **Suggest appropriate resources**: - Consult your primary care physician - Contact a relevant medical specialist - Call a nurse hotline if available - Visit an urgent care or emergency room if urgent 4. **Provide general wellness information** if applicable (without specific medical advice) 5. **Encourage professional consultation** for personalized care +6. **If there are signs of an emergency or self-harm**: + - Urge the user to contact local emergency services immediately (e.g., 911 in the US) or go to the nearest emergency department. + - Do not continue with general wellness information; prioritize the safety message. Your health is important, and qualified medical professionals are best equipped to provide the specific guidance you need. Is there anything else I can help you with that doesn't involve medical advice?`; }If this will run in locales outside the US, consider parameterizing the emergency number in the sample app.
66-74: Tighten typing surface and expose keys for category-safe routingThe categories look good. Expose the key union for ergonomic, type-safe switching and optional runtime immutability.
Apply this diff:
export const PROMPT_CATEGORIES = { PROVIDE_INFO: 'provide' as const, REFUSE_ADVICE: 'refuse' as const, MENTAL_HEALTH: 'mental-health' as const, FITNESS: 'fitness' as const, NUTRITION: 'nutrition' as const, } as const; -export type PromptCategory = typeof PROMPT_CATEGORIES[keyof typeof PROMPT_CATEGORIES]; +export type PromptCategory = typeof PROMPT_CATEGORIES[keyof typeof PROMPT_CATEGORIES]; +export type PromptCategoryKey = keyof typeof PROMPT_CATEGORIES; +// Optional: freeze at runtime to prevent mutation by consumers +Object.freeze(PROMPT_CATEGORIES);If helpful, I can also wire a typed map from categories to prompt builders to prevent mismatches in the sample experiment runner.
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (2)
46-49: Gate HTTP request logging behind a debug flag and avoid dumping full payloads.Unconditional logging of URLs and bodies can leak PII and inflate logs. Prefer debug-only logs and avoid printing raw payloads.
Apply this diff:
async post(path: string, body: Record<string, unknown> | any) { - console.log(`POST ${this.baseUrl}${path}`); - console.log(body); + if (process.env.TRACELOOP_DEBUG_HTTP === '1') { + console.debug(`POST ${this.baseUrl}${path}`); + // Avoid dumping raw payloads; log a lightweight hint instead + try { console.debug('payloadSizeChars', JSON.stringify(body).length); } catch {} + } return await fetch(`${this.baseUrl}${path}`, {async put(path: string, body: Record<string, unknown> | any) { - console.log(`PUT ${this.baseUrl}${path}`); - console.log(body); + if (process.env.TRACELOOP_DEBUG_HTTP === '1') { + console.debug(`PUT ${this.baseUrl}${path}`); + try { console.debug('payloadSizeChars', JSON.stringify(body).length); } catch {} + } return await fetch(`${this.baseUrl}${path}`, {Also applies to: 70-72
21-39: Expose read-only getters for baseUrl, apiKey, and SDK version to avoid private-field indexing elsewhere.Other modules access client['baseUrl'] / client['apiKey'] which breaks encapsulation. Provide getters and switch call sites to them.
Add these getters (outside the changed hunk):
// Inside TraceloopClient class public getBaseUrl(): string { return this.baseUrl; } public getApiKey(): string { return this.apiKey; } public getSdkVersion(): string { return this.version; }Then replace bracket accesses in consumers (e.g., SSE client) with these methods. See suggested diffs in their files.
packages/traceloop-sdk/src/lib/node-server-sdk.ts (1)
29-33: Consider exporting the SSE client publicly (optional).If SSEClient is intended for external consumers, re-export it here for consistency with other clients.
Apply this diff (assuming ./client/stream/index.ts re-exports SSEClient):
export { Dataset, Datasets, Column, Row } from "./client/dataset"; export { Experiment } from "./client/experiment"; export { Evaluator } from "./client/evaluator"; +export { SSEClient } from "./client/stream";packages/sample-app/src/sample_experiment.ts (3)
9-14: Fail fast when TRACELOOP_API_KEY is missing and avoid passing undefined.Prevent silent misconfiguration and clearer DX.
Apply this diff:
- traceloop.initialize({ - appName: "sample_experiment", - apiKey: process.env.TRACELOOP_API_KEY, - disableBatch: true, - traceloopSyncEnabled: true, - }); + const apiKey = process.env.TRACELOOP_API_KEY; + if (!apiKey) { + console.error("TRACELOOP_API_KEY is not set"); + process.exit(1); + } + traceloop.initialize({ + appName: "sample_experiment", + apiKey, + disableBatch: true, + traceloopSyncEnabled: true, + });
85-86: Style nit: avoid breaking property chains after a dot.Minor readability/formatting tweak; keeps Prettier happy in most setups.
Apply this diff:
- const results1 = await client. - experiment.run(medicalTaskRefuseAdvice, { + const results1 = await client.experiment.run(medicalTaskRefuseAdvice, {
92-93: Demonstrate concurrency option in the sample (optional).Showcasing concurrency hints users that it's available.
Apply this diff:
stopOnError: false, waitForResults: true, + concurrency: 5,packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (3)
83-87: Pass evaluator slugs to initialization (optional, if backend expects them).Keeps run metadata consistent with selected evaluators.
Apply this diff:
const experimentResponse = await this.initializeExperiment({ slug: experimentSlug || 'default-experiment', datasetSlug, datasetVersion, + evaluatorSlugs: evaluators.map(e => e.name), });
200-203: Drop noisy response logging or guard it under debug.Leaking Response objects clutters logs.
Apply this diff:
- console.log("response", response); + if (process.env.TRACELOOP_DEBUG_HTTP === '1') { + console.debug("initializeExperiment response status:", response.status); + }
264-276: You validate concurrency but never use it. Consider implementing a bounded worker pool (optional).Current execution is sequential and may be slow. A simple p-limit or manual semaphore would honor the option.
If you want, I can provide a drop-in concurrency utility and wire it into the run loop.
packages/traceloop-sdk/src/lib/client/stream/sse-client.ts (3)
105-106: Avoid poking into TraceloopClient private fields; use read-only getters.Bracket access to private members is a code smell and can break with TS settings. Use getBaseUrl/getApiKey/getSdkVersion accessors.
After adding getters to TraceloopClient, apply this diff:
- const fullUrl = `${this.client['baseUrl']}${url}`; + const fullUrl = `${this.client.getBaseUrl()}${url}`;- const fullUrl = `${this.client['baseUrl']}${url}`; + const fullUrl = `${this.client.getBaseUrl()}${url}`; const eventSource = new EventSource(fullUrl, { headers: { - ...options.headers, - 'Authorization': `Bearer ${this.client['apiKey']}`, - 'X-Traceloop-SDK-Version': this.client['version'] + ...options.headers, + 'Authorization': `Bearer ${this.client.getApiKey()}`, + 'X-Traceloop-SDK-Version': this.client.getSdkVersion() } });- const fullUrl = `${this.client['baseUrl']}${url}`; + const fullUrl = `${this.client.getBaseUrl()}${url}`; @@ - 'Authorization': `Bearer ${this.client['apiKey']}`, - 'X-Traceloop-SDK-Version': this.client['version'], + 'Authorization': `Bearer ${this.client.getApiKey()}`, + 'X-Traceloop-SDK-Version': this.client.getSdkVersion(),Also applies to: 167-174, 230-247
16-21: Reduce noisy console logging or guard under a DEBUG flag.The SSE client logs a lot (module load status, every line, content types, etc.). This will overwhelm consumer logs.
- Wrap logs with if (process.env.TRACELOOP_DEBUG_SSE === '1') { … }.
- Prefer console.debug over console.log/warn for non-errors.
- Consider truncating logged payload slices further or removing per-line logs in the fetch path.
Also applies to: 35-49, 231-236, 251-253, 267-297
185-200: Node eventsource error/timeout handling: ensure closure symmetry.onerror sets finished = true but doesn’t close; timeout handler closes, finally closes again — okay but consider closing in onerror for consistency and to free resources sooner.
Apply this diff:
eventSource.onerror = () => { error = new Error('SSE stream error'); finished = true; + try { eventSource.close(); } catch {} };packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (5)
104-115: Avoid reaching into private client fields; prefer using the client’s HTTP helpersAccessing
this.client['baseUrl']andthis.client['apiKey']breaks encapsulation and TypeScript safety. It also duplicates header logic present inTraceloopClient.get.Consider:
- Exposing a typed helper on
TraceloopClient(e.g.,getWithTimeout(path: string, opts?: { signal?: AbortSignal })) and use it here; or- Use
this.client.get()with a resolved path and handle timeout viaAbortController+Promise.race(request won’t abort but you can fail fast).If you prefer a minimal change here, at least compute URLs safely (handle whether
streamUrlalready includes/v2) and don’t access private fields via bracket notation. I can draft a smallresolvePathhelper on the client if you want.Also applies to: 117-117
124-129: Useresponse.json()and reduce noisy loggingYou can simplify and avoid double parsing/logging. Current logging of fully parsed payloads can leak payload details in production logs.
Apply this diff:
- const responseText = await response.text(); - console.log('waitForResult - Response text length:', responseText.length); - - const responseData = JSON.parse(responseText); - console.log('waitForResult - Parsed response:', responseData); + const responseData = await response.json();Optionally gate logs behind a debug flag:
- console.log('waitForResult called with:', { executionId, streamUrl }); + if (process.env.TRACELOOP_DEBUG === '1') { + console.log('waitForResult called with:', { executionId, streamUrl }); + }
161-167: Tighten validation messages for clarity (nit)Messages refer to “At least one evaluator/result” but these are single objects. Suggest wording tweaks.
Apply this diff:
- if (!evaluator) { - throw new Error('At least one evaluator must be specified'); - } + if (!evaluator) { + throw new Error('Evaluator must be specified'); + } ... - if (!taskResult) { - throw new Error('At least one task result must be provided'); - } + if (!taskResult) { + throw new Error('Task result must be provided'); + }Also applies to: 169-178
183-191:createInputSchemaMappinglikely produces incorrect values for objects
String(value)turns objects/arrays into “[object Object]”. If the API expects actual literals or value strings, this loses information. If it expects path-like sources, passing literal values is likely incorrect.If literals are accepted, at least stringify objects:
- for (const [key, value] of Object.entries(input)) { - mapping[key] = { source: String(value) }; - } + for (const [key, value] of Object.entries(input)) { + let source: string; + if (value === null || value === undefined) { + source = ''; + } else if (typeof value === 'object') { + source = JSON.stringify(value); + } else { + source = String(value); + } + mapping[key] = { source }; + }If the API expects “source paths” (e.g.,
task.output.prompt), we should instead map to paths, not literal values. Please confirm the expected contract, and I can adjust accordingly.
25-33: Maketimeoutoptional to match defaulting behavior
runExperimentEvaluatorprovides a default fortimeout, but theEvaluatorRunOptionsinterface currently marks it as required. This forces callers to pass it, defeating the default.See change proposed in
interfaces/evaluator.interface.tsto maketimeout?: number;.packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (2)
24-31: Consider using an ISO string fortimestampor documenting units
timestamp?: numberis ambiguous (ms vs s). Elsewhere in the API you use ISO strings (e.g.,created_at: string). Aligning to ISO 8601 strings improves consistency and reduces unit errors.Apply this diff if you prefer ISO strings:
- timestamp?: number; + // ISO 8601 string (e.g., 2025-08-22T07:15:34Z) + timestamp?: string;Alternatively, keep number but document units explicitly (e.g., epoch ms).
33-39:errors: string[]is lossy; consider a typed error objectHaving only strings makes correlating errors to tasks harder. Consider including
taskIdand optional metadata.Example:
-export interface ExperimentRunResult { - results: TaskResponse[]; - errors: string[]; +export interface ExperimentRunResult { + results: TaskResponse[]; + errors: Array<{ message: string; taskId?: string; code?: string }>; experimentId?: string; runId?: string; evaluations?: ExecutionResponse[]; }Non-breaking alternative: add a new optional
errorDetailsarray and keeperrorsfor backwards compatibility.packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (2)
91-97: ClarifyInputExtractorsemantics (path vs literal)Right now
InputExtractoronly allows{ source: string }, but the SDK builds it from raw values. Please document whethersourceis a value literal or a path into a prior result/dataset. If literals are intended, consider a more expressive type to prevent misuse.Potential extension (backwards compatible):
-export interface InputExtractor { - source: string; -} +export type InputExtractor = + | { source: string } // path, e.g., "task.output.summary" + | { literal: string }; // direct valueIf you approve, I’ll update
createInputSchemaMappingaccordingly.
62-68: Double-checkStreamResultEventpayload shape
data.resultis typed asEvaluatorResult, which itself includesevaluatorName,taskId, etc. Given you already havetaskIdandevaluatorNameat the top-level ofdata, consider whetherresultshould only hold the evaluator’s domain result (e.g.,{ score, metadata }) to avoid duplication.If duplication is intended for convenience, no action needed. Otherwise, refine
EvaluatorResultto the minimal result payload and keep IDs at the event level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.gitignore(2 hunks)CLAUDE.md(1 hunks)packages/sample-app/package.json(2 hunks)packages/sample-app/src/medical_prompts.ts(1 hunks)packages/sample-app/src/sample_experiment.ts(1 hunks)packages/traceloop-sdk/package.json(1 hunks)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts(1 hunks)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts(1 hunks)packages/traceloop-sdk/src/lib/client/evaluator/index.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/index.ts(1 hunks)packages/traceloop-sdk/src/lib/client/stream/index.ts(1 hunks)packages/traceloop-sdk/src/lib/client/stream/sse-client.ts(1 hunks)packages/traceloop-sdk/src/lib/client/traceloop-client.ts(3 hunks)packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/index.ts(1 hunks)packages/traceloop-sdk/src/lib/node-server-sdk.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (1)
InputSchemaMapping(95-97)
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (1)
packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
EvaluatorDetails(5-8)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-94)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
Evaluator(13-193)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (1)
Datasets(10-106)packages/traceloop-sdk/src/lib/utils/response-transformer.ts (1)
transformApiResponse(50-52)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (9)
ExperimentTaskFunction(1-3)ExperimentRunOptions(10-20)ExperimentRunResult(33-39)TaskResponse(24-31)ExecutionResponse(75-78)CreateTaskResponse(85-87)CreateTaskRequest(80-83)InitExperimentRequest(41-50)ExperimentInitResponse(70-73)
packages/traceloop-sdk/src/lib/client/stream/sse-client.ts (3)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-94)packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (3)
SSEClientOptions(45-49)StreamEvent(3-9)SSEStreamEvent(89-89)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
ExecutionResponse(75-78)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (3)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-94)packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (4)
EvaluatorRunOptions(11-19)TriggerEvaluatorRequest(31-38)TriggerEvaluatorResponse(40-43)InputSchemaMapping(95-97)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
ExecutionResponse(75-78)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (2)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (1)
Experiment(17-279)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
Evaluator(13-193)
🪛 LanguageTool
CLAUDE.md
[grammar] ~22-~22: There might be a mistake here.
Context: ...pm nx affected -t build ``` ### Testing Each package has its own test command: `...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ... ## Architecture ### Monorepo Structure - Lerna + Nx: Manages multiple packages ...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...ackages #### traceloop-sdk (Main SDK) - Path: packages/traceloop-sdk/ - **Ex...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ... traceloop-sdk (Main SDK) - Path: packages/traceloop-sdk/ - Exports: @traceloop/node-server-sdk ...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...packages/traceloop-sdk/- **Exports**:@traceloop/node-server-sdk` - Purpose: Primary entry point that orch...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...t that orchestrates all instrumentations - Key Files: - `src/lib/tracing/decora...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...es all instrumentations - Key Files: - src/lib/tracing/decorators.ts: Workflow and task decorators (`@workfl...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...orators (@workflow, @task, @agent) - src/lib/tracing/tracing.ts: Core tracing utilities and span manage...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...re tracing utilities and span management - src/lib/node-server-sdk.ts: Main initialization logic #### Instru...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...ion logic #### Instrumentation Packages Each follows the pattern: `packages/inst...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ...tion Packages Each follows the pattern: packages/instrumentation-[provider]/ - OpenAI: `@traceloop/instrumentation-op...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...trumentation-[provider]/- **OpenAI**:@traceloop/instrumentation-openai- **Anthropic**:@traceloop/instrumentation...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...nstrumentation-openai- **Anthropic**:@traceloop/instrumentation-anthropic - **Bedrock**:@traceloop/instrumentation-b...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...rumentation-anthropic - **Bedrock**:@traceloop/instrumentation-bedrock` - Vector DBs: Pinecone, Chroma, Qdrant p...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...DBs**: Pinecone, Chroma, Qdrant packages - Frameworks: LangChain, LlamaIndex pack...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...# ai-semantic-conventions - Path: packages/ai-semantic-conventions/ - Purpose: OpenTelemetry semantic conven...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...ry semantic conventions for AI/LLM spans - Key File: src/SemanticAttributes.ts ...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...e constants ### Instrumentation Pattern All instrumentations extend `Instrumenta...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...from@opentelemetry/instrumentation`: 1. Hook Registration: Wrap target library...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...**: Wrap target library functions using InstrumentationModuleDefinition 2. Span Creation: Create spans with appro...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...ans with appropriate semantic attributes 3. Data Extraction: Extract request/respo...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...ct request/response data and token usage 4. Error Handling: Capture and record err...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...rors appropriately ### Testing Strategy - Polly.js: Records HTTP interactions fo...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...Key Patterns ### Workspace Dependencies Packages reference each other using `wor...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...ons }); ``` ### Telemetry Configuration - Anonymous telemetry enabled by default -...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...pment Tasks ### Adding New LLM Provider 1. Create new instrumentation package in `p...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: .... Create new instrumentation package in packages/instrumentation-[provider]/ 2. Implement instrumentation extending `Ins...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ... 2. Implement instrumentation extending InstrumentationBase 3. Add to main SDK dependencies in `package...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ...ase3. Add to main SDK dependencies inpackages/traceloop-sdk/package.json` 4. Register in SDK initialization ### Runn...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...ern" ### Debugging Instrumentations Enable OpenTelemetry debug logging:b...
(QB_NEW_EN)
🪛 ESLint
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
[error] 97-97: 'evaluationResults' is never reassigned. Use 'const' instead.
(prefer-const)
packages/traceloop-sdk/src/lib/client/stream/sse-client.ts
[error] 57-57: Type number trivially inferred from a number literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts
[error] 96-96: Type number trivially inferred from a number literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
packages/traceloop-sdk/src/lib/client/dataset/datasets.ts
[error] 81-81: 'url' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Actions: CI
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
[error] 97-97: Step: nx run @traceloop/node-server-sdk:lint - ESLint error: 'evaluationResults' is never reassigned. Use 'const' instead. (prefer-const).
packages/traceloop-sdk/src/lib/client/stream/sse-client.ts
[error] 57-57: Step: nx run @traceloop/node-server-sdk:lint - ESLint error: Type number trivially inferred from a number literal. Remove the explicit type annotation. (no-inferrable-types).
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts
[error] 96-96: Step: nx run @traceloop/node-server-sdk:lint - ESLint error: Type number trivially inferred from a number literal. Remove the explicit type annotation. (no-inferrable-types).
packages/traceloop-sdk/src/lib/client/dataset/datasets.ts
[error] 81-81: Step: nx run @traceloop/node-server-sdk:lint - ESLint error: 'url' is never reassigned. Use 'const' instead. (prefer-const).
🔇 Additional comments (10)
.gitignore (1)
52-54: Nice: ignore local Claude artifactsAdding
.claudeis sensible to keep tooling artifacts out of VCS.packages/sample-app/package.json (1)
76-76: Confirm Node compatibility for eventsource in the sample appSame note as the SDK: ensure
eventsource@^3.0.2is compatible with your Node baseline when running samples locally/CI.You can reuse the engines check script from the SDK comment.
packages/traceloop-sdk/src/lib/client/evaluator/index.ts (1)
1-1: LGTM: clean barrel exportThe named re-export is straightforward and aligns with usage via the client. No issues.
packages/traceloop-sdk/src/lib/client/stream/index.ts (1)
1-1: LGTM: exposes SSEClient via public indexMatches the pattern used elsewhere and keeps import paths stable.
packages/traceloop-sdk/src/lib/client/experiment/index.ts (1)
1-1: LGTM: Experiment barrel exportConsistent with other client barrels; no concerns.
packages/traceloop-sdk/src/lib/interfaces/index.ts (1)
6-7: No duplicate or default exports found in interfaces/I verified the
packages/traceloop-sdk/src/lib/interfacesdirectory and confirmed:
- No duplicate named exports across the individual
.tsinterface files- No files using
export defaultThe existing
export *inindex.tstherefore safely exposes all interface types without risk of collisions or missing defaults.packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
43-45: Good: public surface now exposes Experiment and Evaluator.Makes usage ergonomics better (e.g., client.experiment.run). No issues spotted.
packages/traceloop-sdk/src/lib/node-server-sdk.ts (2)
20-27: Interfaces re-export looks good.Public API now includes experiment types and evaluator stream event types. This aligns with new features.
31-32: Exporting Experiment and Evaluator from the Node SDK is appropriate.This matches the new client surface.
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
83-86: Harden response mapping (camelCasevssnake_case)Depending on your API,
handleResponsemay returnsnake_casekeys (e.g.,execution_id,stream_url). The current code assumescamelCase. This can returnundefinedvalues.Apply this diff to support both shapes and validate presence:
- return { - executionId: data.executionId, - streamUrl: data.streamUrl - }; + const executionId = data.executionId ?? data.execution_id; + const streamUrl = data.streamUrl ?? data.stream_url; + if (!executionId || !streamUrl) { + throw new Error('Malformed trigger response: missing executionId or streamUrl'); + } + return { executionId, streamUrl };⛔ Skipped due to learnings
Learnt from: galzilber PR: traceloop/openllmetry-js#643 File: packages/traceloop-sdk/test/datasets-final.test.ts:97-105 Timestamp: 2025-08-12T13:57:05.901Z Learning: The traceloop-sdk uses a response transformer (`transformApiResponse` in `packages/traceloop-sdk/src/lib/utils/response-transformer.ts`) that converts snake_case API responses to camelCase for SDK interfaces. Raw API responses use snake_case but SDK consumers see camelCase fields.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 81bc188 in 45 seconds. Click for details.
- Reviewed
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/package.json:23
- Draft comment:
Upgraded Node engine requirement to >=18.0.0. Ensure this change is clearly documented to warn users about breaking compatibility with older Node versions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts:18
- Draft comment:
Changing 'timeout' to optional is appropriate, but ensure any consumers handle undefined values gracefully. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_1L8EGVV3kIycd0Mt
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (1)
85-88: ESLint prefer-const on url — already fixedPrevious feedback to use
constforurlhas been addressed. Nothing further to do here.packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
66-73: Guard against missing evaluator to prevent runtime crash
evaluatorisn’t validated before dereferencingevaluator.name. If it’s missing, this will throw. The error message already claims it’s required.Apply this diff:
- if (!experimentId || !taskResult) { - throw new Error('experimentId, evaluator, and taskResult are required'); - } - - if (!evaluator.name) { - throw new Error('evaluator.name is required'); - } + if (!experimentId || !evaluator || !taskResult) { + throw new Error('experimentId, evaluator, and taskResult are required'); + } + if (!evaluator.name || typeof evaluator.name !== 'string' || !evaluator.name.trim()) { + throw new Error('evaluator.name is required and must be a non-empty string'); + }packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (2)
99-140: Remove debug truncation and add per-row error handling honoring stopOnError
rows.slice(0, 2)is a debug leftover and silently drops data. Also, failures intask,createTask, or evaluator runs aren’t recorded totaskErrors.Apply this diff:
- const rows_debug = rows.slice(0, 2); // TODO nina - for (const row of rows_debug) { - const taskOutput = await task(row as TInput); + const { stopOnError = false } = options; + for (const row of rows) { + let taskOutput: Record<string, any>; + try { + taskOutput = await task(row as TInput) as Record<string, any>; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Task execution failed for row ${row?.id ?? 'unknown'}: ${msg}`); + if (stopOnError) break; + continue; + } // Create TaskResponse object const taskResponse: TaskResponse = { input: row, output: taskOutput as Record<string, any>, metadata: { rowId: row.id, timestamp: Date.now() }, timestamp: Date.now() }; // Add to results array taskResults.push(taskResponse); // Create task - const response = await this.createTask( - experimentSlug, - experimentResponse.run.id, - row, - taskOutput as Record<string, any> - ); + let response: CreateTaskResponse; + try { + response = await this.createTask( + experimentSlug, + experimentResponse.run.id, + row, + taskOutput as Record<string, any> + ); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Create task failed for row ${row?.id ?? 'unknown'}: ${msg}`); + if (stopOnError) break; + continue; + } const taskId = response.id; if (evaluators.length > 0) { for (const evaluator of evaluators) { - const singleEvaluationResult = await this.evaluator.runExperimentEvaluator({ - experimentId: experimentResponse.experiment.id, - experimentRunId: experimentResponse.run.id, - taskId, - evaluator, - taskResult: taskOutput as Record<string, any>, - waitForResults, - timeout: 120000 // 2 minutes default - }); - evaluationResults.push(...singleEvaluationResult); + try { + const singleEvaluationResult = await this.evaluator.runExperimentEvaluator({ + experimentId: experimentResponse.experiment.id, + experimentRunId: experimentResponse.run.id, + taskId, + evaluator, + taskResult: taskOutput as Record<string, any>, + waitForResults, + timeout: 120000 // 2 minutes default + }); + evaluationResults.push(...singleEvaluationResult); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Evaluator '${evaluator.name}' failed for row ${row?.id ?? 'unknown'}: ${msg}`); + if (stopOnError) break; + } } } }
246-251: Avoid constructing invalid version URLs; default or validate version
datasetVersion || ''produces/versions//jsonlwhendatasetVersionis absent, which will fail downstream. Either require a version or default to a supported alias like"latest".Apply this diff (if the backend supports a “latest” alias):
- const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, datasetVersion || ''); + const version = datasetVersion ?? 'latest'; + const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, version);If
"latest"is not supported, we should:
- add a
getLatestVersionAsJsonl(slug: string)helper inDatasets, or- throw early when
datasetVersionis missing with a clear message.To quickly scan for any existing “latest” usage, you can run:
#!/bin/bash # Check for "latest" convention in dataset version routes rg -nP --type=ts "/versions/\\$\\{?version|/versions/latest|latest\\b" packages
🧹 Nitpick comments (8)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
186-194: Confirm InputSchemaMapping semantics; String(value) may lose structureCoercing every value to a string can discard structured data and types. If the evaluator expects field references (e.g., JSONPath or named inputs) rather than literal strings, this mapping may be incorrect.
Would you confirm the expected schema of
InputExtractor? If it should reference fields, consider:- mapping[key] = { source: String(value) }; + mapping[key] = { source: `$input.${key}` };Or, if literals are intended, ensure non-string primitives are serialized explicitly and arrays/objects are handled predictably.
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (7)
22-26: Avoid duplicating client sub-clients; consider dependency injection or reorderingYou instantiate fresh
EvaluatorandDatasetsinstances even thoughTraceloopClientalready exposes them. This duplicates state/config and may drift if client wiring changes. Ideally, use the instances fromclient.Options:
- Pass
evaluatoranddatasetsintoExperiment’s constructor (DI), or- Reorder fields in
TraceloopClientsoevaluatoris constructed beforeexperimentand then referenceclient.evaluator/client.datasetshere, or- Make
Experimentextend a common base and accept a shared helper that provides accessors.No change required for this PR, but worth tracking.
28-56: Duplicate response handling logic
ExperimentreimplementshandleResponselogic already present inBaseDatasetEntity. Prefer extracting this into a shared utility (or haveExperimentextend the same base) to avoid drift in error handling and content-type quirks.
76-88: Throwing on missing slug but still passing a fallback slug is inconsistentYou throw when
!experimentSlugand then passexperimentSlug || 'default-experiment'toinitializeExperiment. The fallback is dead code.Apply this diff:
- const experimentResponse = await this.initializeExperiment({ - slug: experimentSlug || 'default-experiment', + const experimentResponse = await this.initializeExperiment({ + slug: experimentSlug, datasetSlug, datasetVersion, });
200-205: Remove noisy response log
console.log("response", response);will dump a Response object into user logs. Remove or gate behind a debug flag.Apply this diff:
- console.log("response", response);
212-231: Comment is correct — first line is a columns definition in your custom JSONLAcknowledging the custom format (first line = columns). Consider updating the comment to explicitly call this out for future maintainers.
Suggested doc-only tweak:
- * Parse JSONL string into list of {col_name: col_value} dictionaries - * Skips the first line (columns definition) + * Parse Traceloop JSONL into {col_name: col_value} rows. + * Note: In Traceloop’s dataset format, line 1 contains column definitions and is intentionally skipped.
264-277: Concurrency option is validated but unusedYou validate
concurrencybut don’t apply it. Consider implementing a simple limiter (e.g., batch processing) or deferring the option until it’s wired.Happy to propose a minimal in-house limiter without adding dependencies if you want it in this PR.
90-96: SDK logging: gate progress logs behind a debug flagThe emoji logs are helpful during development but noisy in libraries. Consider gating them, e.g.,
const DEBUG = process.env.TRACELOOP_DEBUG === '1'; if (DEBUG) console.log(`🔧 Step 2: Getting dataset rows for: ${datasetSlug}, version: ${datasetVersion}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/sample-app/package.json(2 hunks)packages/traceloop-sdk/package.json(2 hunks)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts(1 hunks)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/stream/sse-client.ts(1 hunks)packages/traceloop-sdk/src/lib/client/traceloop-client.ts(2 hunks)packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/sample-app/package.json
- packages/traceloop-sdk/package.json
- packages/traceloop-sdk/src/lib/client/traceloop-client.ts
- packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts
- packages/traceloop-sdk/src/lib/client/stream/sse-client.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T12:44:43.829Z
Learnt from: nina-kollman
PR: traceloop/openllmetry-js#672
File: packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:212-231
Timestamp: 2025-08-22T12:44:43.829Z
Learning: In the Traceloop SDK experiment system, JSONL datasets use a custom format where the first line contains column definitions and subsequent lines contain the actual data rows. The parseJsonlToRows method in packages/traceloop-sdk/src/lib/client/experiment/experiment.ts intentionally skips the first line (index 0) to avoid parsing the column definition as data.
Applied to files:
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
🧬 Code graph analysis (2)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-89)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
Evaluator(13-196)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (1)
Datasets(10-110)packages/traceloop-sdk/src/lib/utils/response-transformer.ts (1)
transformApiResponse(50-52)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (9)
ExperimentTaskFunction(1-3)ExperimentRunOptions(10-20)ExperimentRunResult(33-39)TaskResponse(24-31)ExecutionResponse(75-78)CreateTaskResponse(85-87)CreateTaskRequest(80-83)InitExperimentRequest(41-50)ExperimentInitResponse(70-73)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (3)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-89)packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (4)
EvaluatorRunOptions(11-19)TriggerEvaluatorRequest(31-38)TriggerEvaluatorResponse(40-43)InputSchemaMapping(95-97)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
ExecutionResponse(75-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (1)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
22-56: Evaluator run flow looks solidClear orchestration and a sensible immediate-return path when
waitForResultsis false.
| if (!version || version === "") { | ||
| throw new Error("Version is required"); | ||
| } | ||
|
|
||
| const url = `/v2/datasets/${slug}/versions/${version}/jsonl`; | ||
|
|
||
| const response = await this.client.get(url); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate slug and align version guard with existing CSV method
- Missing
this.validateDatasetSlug(slug)here (present ingetVersionCSV). Add it for consistency and early input validation. - Align the version check/error message with
getVersionCSVand guard against whitespace-only values.
Apply this diff:
async getVersionAsJsonl(slug: string, version: string): Promise<string> {
- if (!version || version === "") {
- throw new Error("Version is required");
- }
+ this.validateDatasetSlug(slug);
+ if (!version || version.trim() === "") {
+ throw new Error("Version must be a non-empty string");
+ }
const url = `/v2/datasets/${slug}/versions/${version}/jsonl`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!version || version === "") { | |
| throw new Error("Version is required"); | |
| } | |
| const url = `/v2/datasets/${slug}/versions/${version}/jsonl`; | |
| const response = await this.client.get(url); | |
| async getVersionAsJsonl(slug: string, version: string): Promise<string> { | |
| this.validateDatasetSlug(slug); | |
| if (!version || version.trim() === "") { | |
| throw new Error("Version must be a non-empty string"); | |
| } | |
| const url = `/v2/datasets/${slug}/versions/${version}/jsonl`; | |
| const response = await this.client.get(url); | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In packages/traceloop-sdk/src/lib/client/dataset/datasets.ts around lines 81 to
87, add a call to this.validateDatasetSlug(slug) before building the URL and
change the version guard to mirror getVersionCSV by checking for missing or
whitespace-only values (e.g., if (!version || version.trim() === "") ) and throw
the same error message used in getVersionCSV; this ensures early slug validation
and consistent version validation/error text.
| console.log('waitForResult called with:', { executionId, streamUrl }); | ||
|
|
||
| const fullStreamUrl = `${this.client['baseUrl']}/v2${streamUrl}`; | ||
| console.log('Full stream URL:', fullStreamUrl); | ||
|
|
||
| try { | ||
| const response = await fetch(fullStreamUrl, { | ||
| headers: { | ||
| 'Authorization': `Bearer ${this.client['apiKey']}`, | ||
| 'Accept': 'application/json', | ||
| 'Cache-Control': 'no-cache', | ||
| }, | ||
| }); | ||
|
|
||
| console.log('waitForResult - Response status:', response.status, response.statusText); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| throw new Error(`Failed to get results: ${response.status}, body: ${errorText}`); | ||
| } | ||
|
|
||
| const responseText = await response.text(); | ||
| console.log('waitForResult - Response text length:', responseText.length); | ||
|
|
||
| const responseData = JSON.parse(responseText); | ||
| console.log('waitForResult - Parsed response:', responseData); | ||
|
|
||
| // Check execution ID match | ||
| if (responseData.execution_id && responseData.execution_id !== executionId) { | ||
| throw new Error(`Execution ID mismatch: ${responseData.execution_id} !== ${executionId}`); | ||
| } | ||
|
|
||
| // Convert to ExecutionResponse format | ||
| const executionResponse: ExecutionResponse = { | ||
| executionId: responseData.execution_id, | ||
| result: responseData.result | ||
| }; | ||
|
|
||
| return [executionResponse]; | ||
|
|
||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid private field access, implement timeout, and normalize the stream URL
- Don’t reach into
this.client['baseUrl']orthis.client['apiKey']; usethis.client.get(...)for auth/headers and to keep encapsulation intact. - Implement the
timeoutparameter (currently unused) to avoid indefinite hangs. - Normalize
streamUrlto avoid accidental/v2/v2duplication. - Reduce noisy console logging in SDK code; prefer a debug mode if needed.
Apply this diff:
- console.log('waitForResult called with:', { executionId, streamUrl });
-
- const fullStreamUrl = `${this.client['baseUrl']}/v2${streamUrl}`;
- console.log('Full stream URL:', fullStreamUrl);
-
- try {
- const response = await fetch(fullStreamUrl, {
- headers: {
- 'Authorization': `Bearer ${this.client['apiKey']}`,
- 'Accept': 'application/json',
- 'Cache-Control': 'no-cache',
- },
- });
-
- console.log('waitForResult - Response status:', response.status, response.statusText);
+ try {
+ const path = streamUrl.startsWith('/v2/')
+ ? streamUrl
+ : `/v2${streamUrl}`;
+
+ // Enforce timeout without relying on AbortSignal.timeout (Node 14 compat)
+ const response = (await Promise.race([
+ this.client.get(path),
+ new Promise<Response>((_, reject) =>
+ setTimeout(() => reject(new Error(`Timed out after ${timeout}ms`)), timeout)
+ ),
+ ])) as Response;
if (!response.ok) {
const errorText = await response.text();
throw new Error(`Failed to get results: ${response.status}, body: ${errorText}`);
}
- const responseText = await response.text();
- console.log('waitForResult - Response text length:', responseText.length);
-
- const responseData = JSON.parse(responseText);
- console.log('waitForResult - Parsed response:', responseData);
+ // Use shared response handler to JSON-parse and camelCase keys
+ const data = await this.handleResponse(response);
// Check execution ID match
- if (responseData.execution_id && responseData.execution_id !== executionId) {
- throw new Error(`Execution ID mismatch: ${responseData.execution_id} !== ${executionId}`);
+ if (data.executionId && data.executionId !== executionId) {
+ throw new Error(`Execution ID mismatch: ${data.executionId} !== ${executionId}`);
}
// Convert to ExecutionResponse format
const executionResponse: ExecutionResponse = {
- executionId: responseData.execution_id,
- result: responseData.result
+ executionId: data.executionId ?? executionId,
+ result: data.result ?? {}
};
return [executionResponse];If you want structured debug logs, consider gating them behind an env flag:
const DEBUG = process.env.TRACELOOP_DEBUG === '1';
// ...
if (DEBUG) console.debug('waitForResult...', { executionId, streamUrl });🤖 Prompt for AI Agents
In packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts around lines
106–146, replace direct private field access to this.client['baseUrl'] and
this.client['apiKey'] by using the client’s public request/get method to obtain
headers/auth and to perform the fetch; implement and honor the existing timeout
parameter by racing the fetch against a timeout promise and rejecting when
elapsed; normalize streamUrl to strip any leading /v2 or leading slash before
prefixing with the client’s v2 path to avoid duplicate /v2/v2 sequences; remove
noisy console.log calls and instead gate any structured debug logs behind a
TRACELOOP_DEBUG env flag (e.g. if (DEBUG) console.debug(...)); keep the existing
response error handling, JSON parse, execution_id check, and return structure
intact.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 711962a in 2 minutes and 3 seconds. Click for details.
- Reviewed
247lines of code in8files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/sample-app/src/sample_experiment.ts:90
- Draft comment:
Commenting out the static experimentSlug seems intentional to use auto-generation. Consider adding a clarifying comment so users understand that a slug will be auto-generated if none is provided. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment makes an assumption about auto-generation behavior that isn't proven in the code. We don't see the implementation of the experiment.run() function or its type definitions. The experimentSlug could be required and this could be a bug. Without seeing the underlying implementation, we can't be confident about the behavior. I could be wrong about auto-generation - maybe this is standard documented behavior of the SDK that an experienced developer would know. The comment being added could be helpful documentation. Even if auto-generation is standard behavior, we should follow the principle of only keeping comments we're very confident about. Making assumptions about SDK behavior without clear evidence violates this principle. Delete the comment since it makes assumptions about behavior that isn't clearly evidenced in the code we can see.
2. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:31
- Draft comment:
The generateExperimentSlug function truncates the generated slug to 15 characters. Consider assessing the risk of collisions in high-volume usage or adopting a more robust unique ID generation strategy. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The collision risk is real but likely very low - timestamp in base36 provides good uniqueness, and 5 random chars adds entropy. The 15 char limit seems reasonable for slugs. More importantly, this is speculative - there's no evidence of actual collisions being a problem. The comment suggests "considering" rather than pointing out a definite issue. The current implementation could be sufficient for the actual usage patterns. Without knowing the volume of experiments or seeing evidence of collisions, I may be too quick to dismiss the comment. While collisions are theoretically possible, the comment is speculative and doesn't point to a concrete issue. The current implementation provides reasonable uniqueness for most use cases. Delete the comment because it's speculative ("consider assessing") rather than pointing out a definite issue that needs fixing.
3. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:104
- Draft comment:
The use of 'rows_debug' slicing to the first two rows is likely for testing. Ensure this temporary limitation is removed or properly gated in production code. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/traceloop-sdk/src/lib/client/traceloop-client.ts:26
- Draft comment:
A new 'experimentSlug' property has been added. Ensure it is consistently propagated and documented within the SDK configuration. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the new property is consistently propagated and documented. This falls under asking the author to ensure something is done, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue.
5. packages/traceloop-sdk/src/lib/configuration/index.ts:42
- Draft comment:
The configuration now sets experimentSlug from the TRACELOOP_EXP_SLUG environment variable if not provided. Ensure this behavior is clearly documented so users know which environment variable to set. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the behavior is documented, which falls under the rule of not asking the author to ensure something is done. This is not a specific code suggestion or a request for a test, so it should be removed.
6. packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts:38
- Draft comment:
The type for evaluations has been changed to Record<string, any>[]. Consider using a more specific type (e.g., ExecutionResponse) to improve type safety if appropriate. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The change appears to be intentionally making the type more generic, moving away from the specific ExecutionResponse type. While using more specific types is generally good practice, the author likely had a reason for making this change. Without understanding the broader context of why this change was made, suggesting to revert back to the previous type could be counterproductive. I might be too hesitant - type safety is important and this is a clear regression in type safety that could lead to runtime errors. However, the intentional nature of this change suggests there may be valid reasons for needing the more flexible type, such as supporting different evaluation response formats. Without that context, we can't be certain the suggestion is correct. While the comment raises a valid point about type safety, we should trust that the author had a reason for making the type more generic and not push back without understanding the full context.
Workflow ID: wflow_2iC6FzZEyTCNta3G
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (2)
104-143: Remove debug slice and add per-row error handling honoring stopOnError.Processing only the first two rows is a functional bug and breaks real runs; lack of per-row try/catch means a single task/evaluator failure aborts the entire experiment and the errors array remains unused.
Apply this refactor to iterate all rows, capture errors per stage, and respect stopOnError:
- const rows_debug = rows.slice(0, 2); // TODO nina - for (const row of rows_debug) { - const taskOutput = await task(row as TInput); + const { stopOnError = false } = options; + let shouldBreak = false; + for (const row of rows) { + let taskOutput: Record<string, any>; + try { + taskOutput = await task(row as TInput) as Record<string, any>; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Task execution failed for row '${row?.id ?? "unknown"}': ${msg}`); + if (stopOnError) { shouldBreak = true; break; } + continue; + } // Create TaskResponse object const taskResponse: TaskResponse = { input: row, output: taskOutput as Record<string, any>, metadata: { - rowId: row.id, - timestamp: Date.now() + rowId: row?.id, + timestamp: Date.now() }, timestamp: Date.now() }; taskResults.push(taskResponse); - const response = await this.createTask( - experimentSlug, - experimentResponse.run.id, - row, - taskOutput as Record<string, any> - ); - const taskId = response.id; + let taskId: string | undefined; + try { + const response = await this.createTask( + experimentSlug, + experimentResponse.run.id, + row, + taskOutput as Record<string, any> + ); + taskId = response.id; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`CreateTask failed for row '${row?.id ?? "unknown"}': ${msg}`); + if (stopOnError) { shouldBreak = true; break; } + continue; + } if (evaluators.length > 0) { for (const evaluator of evaluators) { - const singleEvaluationResult = await this.evaluator.runExperimentEvaluator({ - experimentId: experimentResponse.experiment.id, - experimentRunId: experimentResponse.run.id, - taskId, - evaluator, - taskResult: taskOutput as Record<string, any>, - waitForResults, - timeout: 120000 // 2 minutes default - }); - evaluationResults.push(...singleEvaluationResult); + try { + const singleEvaluationResult = await this.evaluator.runExperimentEvaluator({ + experimentId: experimentResponse.experiment.id, + experimentRunId: experimentResponse.run.id, + taskId: taskId!, + evaluator, + taskResult: taskOutput as Record<string, any>, + waitForResults, + timeout: 120000 // 2 minutes default + }); + evaluationResults.push(...singleEvaluationResult); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Evaluator '${evaluator.name}' failed for row '${row?.id ?? "unknown"}': ${msg}`); + if (stopOnError) { shouldBreak = true; break; } + } } + if (shouldBreak) break; } } + if (shouldBreak) { + console.warn("Execution stopped due to stopOnError."); + }
249-251: Avoid constructing invalid URL when datasetVersion is omitted; default to 'latest' or add a dedicated helper.Using
datasetVersion || ''causesgetVersionAsJsonlto throw ("Version is required") and can form/versions//jsonl. Provide a concrete fallback.- const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, datasetVersion || ''); + // If backend supports it, use 'latest' alias; otherwise call a dedicated helper. + const version = datasetVersion ?? 'latest'; + const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, version);If the API does NOT support a 'latest' alias, add a helper in Datasets and call it here:
- const version = datasetVersion ?? 'latest'; - const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, version); + if (!datasetVersion) { + const dataset = await this.datasets.getLatestVersionAsJsonl(datasetSlug); + const rows = this.parseJsonlToRows(dataset); + console.log(`✅ Dataset fetched successfully: ${rows.length} rows`); + return rows; + } + const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, datasetVersion);Please confirm whether the backend supports the 'latest' alias; I can adjust the Datasets class accordingly.
🧹 Nitpick comments (15)
packages/traceloop-sdk/src/lib/interfaces/traceloop-client.interface.ts (1)
5-5: Add brief TSDoc for the new option to keep public API self-documentedA short comment helps consumers understand scope and intended source of truth for the slug (env vs. InitializeOptions vs. per-run options).
- experimentSlug?: string; + /** + * Experiment slug to scope client operations (experiments/evaluations). + * Typically provided via InitializeOptions or TRACELOOP_EXP_SLUG. + * Can be overridden per run via ExperimentRunOptions.experimentSlug. + */ + experimentSlug?: string;packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.ts (1)
139-144: Clarify precedence and expected format for experimentSlugGood addition. Consider documenting:
- Precedence when both InitializeOptions.experimentSlug and ExperimentRunOptions.experimentSlug are provided (per-run likely wins).
- Expected format (e.g., lowercase kebab-case, max length), even if enforcement is in validation.
/** * The experiment slug to use when running experiments. Optional. - * Defaults to the TRACELOOP_EXP_SLUG environment variable. + * Defaults to the TRACELOOP_EXP_SLUG environment variable. + * If also provided in ExperimentRunOptions, the per-run value takes precedence. + * Expected format: lowercase letters, digits and hyphens only (e.g., "my-experiment-1"). */ experimentSlug?: string;If you prefer validation over documentation only, I can add a small check in validateConfiguration() to enforce a slug regex like
/^[a-z0-9]+(?:-[a-z0-9]+)*$/. Want me to draft that?packages/traceloop-sdk/src/lib/configuration/index.ts (1)
42-44: Prefer nullish-coalescing assignment to avoid overriding intentional falsy-but-valid valuesUsing
??=makes the intent explicit: only fall back to the env var when the option isundefinedornull. It also mirrors modern style for the other defaults if you choose to update them later.- if (!options.experimentSlug) { - options.experimentSlug = process.env.TRACELOOP_EXP_SLUG; - } + options.experimentSlug ??= process.env.TRACELOOP_EXP_SLUG;If you want to keep consistency with the existing
!options.baseUrlstyle, no change needed.packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (4)
10-20: Document option precedence and constraints; consider widening aux metadata type
- Add brief TSDoc for each property, especially experimentSlug (overrides InitializeOptions), stopOnError/waitForResults semantics, and concurrency (positive integer, default).
- aux is currently
Record<string, string>. Consider allowing primitive values to accommodate numeric/boolean metadata.export interface ExperimentRunOptions { - datasetSlug?: string; - datasetVersion?: string; - evaluators?: EvaluatorDetails[]; - experimentSlug?: string; - relatedRef?: Record<string, string>; - aux?: Record<string, string>; - stopOnError?: boolean; - waitForResults?: boolean; - concurrency?: number; + /** Dataset slug to run against (optional). */ + datasetSlug?: string; + /** Specific dataset version (optional). */ + datasetVersion?: string; + /** Evaluators to execute for this run (optional). */ + evaluators?: EvaluatorDetails[]; + /** Per-run experiment slug; overrides client/initialize-level slug if provided. */ + experimentSlug?: string; + /** References to related entities (e.g., commit, branch) */ + relatedRef?: Record<string, string>; + /** Arbitrary run metadata. Consider allowing number|boolean if server accepts it. */ + aux?: Record<string, string>; + /** Fail fast on first task error (default: false). */ + stopOnError?: boolean; + /** Block until evaluations complete (default: false). */ + waitForResults?: boolean; + /** Max concurrent task executions (> 0). */ + concurrency?: number; }If
auxcan accept primitive values, we could evolve toRecord<string, string | number | boolean>in a minor release.
24-31: Clarify timestamp unit and snake_case field source
- Please document whether
timestampis epoch milliseconds or seconds.input_schema_mappingis snake_case while the rest of this interface uses camelCase. If this mirrors the wire format from the backend, add a note so consumers don’t attempt to “fix” it downstream.export interface TaskResponse { input: Record<string, any>; output: Record<string, any>; - metadata?: Record<string, any>; - timestamp?: number; + metadata?: Record<string, any>; + /** Unix epoch in milliseconds (backend contract). */ + timestamp?: number; error?: string; - input_schema_mapping?: InputSchemaMapping; + /** Mirrors backend field naming; kept snake_case to match wire format. */ + input_schema_mapping?: InputSchemaMapping; }
33-39: Consider a structured error object for richer diagnosticsA plain
string[]of errors may limit client UX and filtering. If feasible, introduce a lightweight error shape (code/message/context). Backward-compatible path: keeperrors: string[]and adderrorDetails?: Array<{ code?: string; message: string; context?: Record<string, any> }>later.
52-68: Document timestamp format for responses; ensure consistency across shapesThese response types use snake_case and string timestamps. Please clarify if
created_at/updated_atare ISO-8601 strings returned by the backend. Consistency notes help consumers parse correctly.export interface ExperimentResponse { id: string; slug: string; metadata?: Record<string, any>; - created_at: string; - updated_at: string; + /** ISO-8601 timestamp string (from backend). */ + created_at: string; + /** ISO-8601 timestamp string (from backend). */ + updated_at: string; }packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (8)
182-189: Improve createTask error diagnostics.Surface HTTP status and body to aid debugging instead of a generic message.
- if (!response.ok) { - throw new Error(`Failed to create task for experiment '${experimentSlug}'`); - } + if (!response.ok) { + let body = ''; + try { body = await response.text(); } catch {} + throw new Error( + `Failed to create task for experiment '${experimentSlug}': ${response.status} ${response.statusText}${body ? `, body: ${body}` : ''}` + ); + }
205-208: Remove debug logging of raw Response or gate behind a debug flag.Logging the Response object is noisy for consumers and not actionable.
- console.log("response", response); + // Consider enabling via a DEBUG flag if needed: + // if (process.env.TRACELOOP_DEBUG) console.debug("initializeExperiment response", response.status);
74-80: Expose stopOnError and align logs.
- Add
stopOnErrorto destructuring to pair with the per-row error handling change.- Minor nit: The log at Line 252 prints the array; prefer length for readability.
- evaluators = [], - waitForResults = true, + evaluators = [], + waitForResults = true, + stopOnError = false,And later:
- console.log(`✅ Dataset fetched successfully: ${rows || 'unknown'}`); + console.log(`✅ Dataset fetched successfully: ${rows.length} rows`);
259-271: Concurrency is validated but unused.Either implement a concurrency limiter for task/evaluator execution or defer validation until implemented to avoid misleading users.
If you want to keep it simple without adding dependencies, we can implement a lightweight promise pool here; say the word and I’ll provide a focused diff.
37-65: Response handler looks good; consider preserving non-JSON error bodies.Current JSON-error extraction is solid; you might optionally include
response.text()when JSON parsing fails to aid diagnostics.
108-117: Avoid double timestamps and undefined row IDs.You record timestamps in both
metadata.timestampandtimestamp. Unless both are required, keep one. Also,row.idmay be absent in some datasets; guard it.See the main loop refactor above which sets
rowId: row?.idand usesDate.now()once for both fields if you prefer.
241-254: Require datasetVersion explicitly or document fallback behavior.If a dataset version is required by the API, validate it up front here and provide a clear error; otherwise, implement the 'latest' fallback as suggested.
Let me know which behavior the backend guarantees and I’ll wire it accordingly (and add tests).
81-86: LGTM: experimentSlug fallback.Using the client’s
experimentSlugor generating one is reasonable. Consider exposing the generator publicly if consumers need it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/sample-app/src/sample_experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/traceloop-client.ts(3 hunks)packages/traceloop-sdk/src/lib/configuration/index.ts(2 hunks)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/index.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/traceloop-client.interface.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sample-app/src/sample_experiment.ts
- packages/traceloop-sdk/src/lib/client/traceloop-client.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T12:44:43.829Z
Learnt from: nina-kollman
PR: traceloop/openllmetry-js#672
File: packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:212-231
Timestamp: 2025-08-22T12:44:43.829Z
Learning: In the Traceloop SDK experiment system, JSONL datasets use a custom format where the first line contains column definitions and subsequent lines contain the actual data rows. The parseJsonlToRows method in packages/traceloop-sdk/src/lib/client/experiment/experiment.ts intentionally skips the first line (index 0) to avoid parsing the column definition as data.
Applied to files:
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
🧬 Code graph analysis (2)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-96)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
Evaluator(13-196)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (1)
Datasets(10-110)packages/traceloop-sdk/src/lib/utils/response-transformer.ts (1)
transformApiResponse(50-52)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (9)
ExperimentTaskFunction(1-3)ExperimentRunOptions(10-20)ExperimentRunResult(33-39)TaskResponse(24-31)ExecutionResponse(75-78)CreateTaskResponse(85-87)CreateTaskRequest(80-83)InitExperimentRequest(41-50)ExperimentInitResponse(70-73)
packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (1)
InputSchemaMapping(95-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (7)
packages/traceloop-sdk/src/lib/configuration/index.ts (1)
98-103: LGTM: clean propagation of experimentSlug into the clientPassing the slug through the client constructor is straightforward and keeps the singleton semantics intact.
packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (3)
1-3: LGTM: generic task signature is flexible and future-proofAllows sync/async and typed I/O; solid baseline.
75-78: LGTM: ExecutionResponse is minimal and clearNo issues spotted.
80-87: LGTM: Task create request/response shapes are straightforwardGood as-is; keep aligned with backend expectations.
packages/traceloop-sdk/src/lib/interfaces/index.ts (1)
6-7: Expose new Experiment/Evaluator interfaces via the public barrelMakes the new surface discoverable to consumers. Looks good.
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (2)
212-236: JSONL parsing aligns with the custom format.Acknowledging your custom JSONL where the first line is a column definition; skipping index 0 is intentional and correct per retrieved learnings.
100-103: LGTM: prefer-const fix applied.
evaluationResults,taskResults, andtaskErrorsare correctly declared with const since only their contents change.
| constructor(client: TraceloopClient) { | ||
| this.client = client; | ||
| this.evaluator = new Evaluator(client); | ||
| this.datasets = new Datasets(client); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inject Datasets/Evaluator instead of re-instantiating to avoid duplicate instances and ease testing.
Experiment currently creates new instances, while TraceloopClient also constructs them—leading to two sets of singletons and potential state drift.
Prefer dependency injection with reuse from the client:
- constructor(client: TraceloopClient) {
- this.client = client;
- this.evaluator = new Evaluator(client);
- this.datasets = new Datasets(client);
- }
+ constructor(client: TraceloopClient, deps?: { evaluator?: Evaluator; datasets?: Datasets }) {
+ this.client = client;
+ this.evaluator = deps?.evaluator ?? new Evaluator(client);
+ this.datasets = deps?.datasets ?? new Datasets(client);
+ }Then initialize in TraceloopClient (outside this file) as:
- this.userFeedback = new UserFeedback(this);
- this.datasets = new Datasets(this);
- this.experiment = new Experiment(this);
- this.evaluator = new Evaluator(this);
+ this.userFeedback = new UserFeedback(this);
+ this.datasets = new Datasets(this);
+ this.evaluator = new Evaluator(this);
+ this.experiment = new Experiment(this, { evaluator: this.evaluator, datasets: this.datasets });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(client: TraceloopClient) { | |
| this.client = client; | |
| this.evaluator = new Evaluator(client); | |
| this.datasets = new Datasets(client); | |
| } | |
| constructor(client: TraceloopClient, deps?: { evaluator?: Evaluator; datasets?: Datasets }) { | |
| this.client = client; | |
| this.evaluator = deps?.evaluator ?? new Evaluator(client); | |
| this.datasets = deps?.datasets ?? new Datasets(client); | |
| } |
🤖 Prompt for AI Agents
In packages/traceloop-sdk/src/lib/client/experiment/experiment.ts around lines
22 to 26, the constructor re-instantiates Evaluator and Datasets causing
duplicate singleton/state drift; change the constructor to accept Evaluator and
Datasets (or an options object) as injected dependencies and assign those when
provided, falling back only if absolutely necessary, then update TraceloopClient
to pass its existing Evaluator/Datasets instances into the Experiment
constructor so the same instances are reused and easier to test.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 75678eb in 1 minute and 52 seconds. Click for details.
- Reviewed
113lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/sample-app/src/sample_experiment.ts:89
- Draft comment:
Using a string for evaluator ("medical_advice") now aligns with the new union type; ensure the backend doesn’t require a version when using string evaluators. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the backend doesn't require a version when using string evaluators. This is a request for confirmation, which violates the rule against asking the author to confirm their intention or ensure behavior. Therefore, this comment should be removed.
2. packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts:66
- Draft comment:
The error message mentions 'evaluator' even though only experimentId and taskResult are checked; consider updating the message for accuracy. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts:177
- Draft comment:
For consistency with experiment run validation, consider validating that evaluator.version is provided for object evaluators in validateEvaluatorOptions. - Reason this comment was not posted:
Comment was on unchanged code.
4. packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts:112
- Draft comment:
Consider using explicit accessor methods for baseUrl and apiKey instead of bracket notation for improved clarity and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:274
- Draft comment:
Requiring evaluator.version for object evaluators is stricter; ensure all evaluator configurations include a non-empty version and update documentation if needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all evaluator configurations include a non-empty version and to update documentation if needed. This falls under asking the author to ensure something is done, which is against the rules.
6. packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts:9
- Draft comment:
Changing 'version' to a required field in EvaluatorWithVersion is a breaking change; ensure all consumers are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that all consumers are updated due to a breaking change. This falls under the rule of not asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or ask for a specific test to be written.
7. packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts:5
- Draft comment:
There's an extra space betweenEvaluatorDetailsand the assignment operator=on line 5. Consider removing the extra space for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the extra space, this is an extremely minor formatting issue that doesn't affect functionality. Most IDEs and formatters would automatically fix this. It's not worth the cognitive overhead of a PR comment. The rules specifically say not to make obvious or unimportant comments. The extra space could be a sign of inconsistent code formatting practices which might matter for maintainability. Some teams are very strict about formatting. Even if formatting consistency is important, this is better handled by automated formatters and linters rather than manual PR comments. The cognitive cost of the comment outweighs its value. Delete this comment as it points out an extremely minor formatting issue that doesn't warrant a PR comment and would be better handled by automated tools.
Workflow ID: wflow_AUiReKaLf6k0MVFl
You can customize
| throw new Error('Task function is required and must be a function'); | ||
| } | ||
|
|
||
| const { concurrency = 5 } = options; |
There was a problem hiding this comment.
@nirga I can see the code is not actually using this.
I am removing it and we can optimize concurrency later on
| * The experiment slug to use when running experiments. Optional. | ||
| * Defaults to the TRACELOOP_EXP_SLUG environment variable. | ||
| */ | ||
| experimentSlug?: string; |
There was a problem hiding this comment.
@nirga No it is not the prefix.. Its the entire slug
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (3)
22-26: Inject Evaluator/Datasets to avoid duplicate instances and ease testingRe-instantiating
EvaluatorandDatasetshere duplicates the instances already created inTraceloopClient, which can cause state drift and complicate testing/mocking. Prefer dependency injection with reuse from the client.Apply this diff:
- constructor(client: TraceloopClient) { - this.client = client; - this.evaluator = new Evaluator(client); - this.datasets = new Datasets(client); - } + constructor(client: TraceloopClient, deps?: { evaluator?: Evaluator; datasets?: Datasets }) { + this.client = client; + this.evaluator = deps?.evaluator ?? new Evaluator(client); + this.datasets = deps?.datasets ?? new Datasets(client); + }And initialize from
TraceloopClient:// packages/traceloop-sdk/src/lib/client/traceloop-client.ts this.datasets = new Datasets(this); this.evaluator = new Evaluator(this); this.experiment = new Experiment(this, { evaluator: this.evaluator, datasets: this.datasets });
241-254: Avoid constructing a broken versions URL when datasetVersion is omitted
datasetVersion || ''yields/versions//jsonland will reliably fail becausegetVersionAsJsonlthrows on empty version. Choose one of the following:Option A — Default to “latest” (if the backend supports it):
- const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, datasetVersion || ''); - const rows = this.parseJsonlToRows(dataset); - console.log(`✅ Dataset fetched successfully: ${rows || 'unknown'}`); + const version = datasetVersion ?? 'latest'; + const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, version); + const rows = this.parseJsonlToRows(dataset); + console.log(`✅ Dataset fetched successfully: ${rows.length} rows`);Option B — Require
datasetVersionexplicitly (aligns with Datasets API check):- console.log(`🔧 Fetching dataset: ${datasetSlug}`); - const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, datasetVersion || ''); + if (!datasetVersion) { + throw new Error('Dataset version is required for experiment execution'); + } + console.log(`🔧 Fetching dataset: ${datasetSlug}, version: ${datasetVersion}`); + const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, datasetVersion); const rows = this.parseJsonlToRows(dataset); - console.log(`✅ Dataset fetched successfully: ${rows || 'unknown'}`); + console.log(`✅ Dataset fetched successfully: ${rows.length} rows`);Please confirm whether
/v2/datasets/{slug}/versions/latest/jsonlis supported before choosing Option A.
100-143: Remove debug row slicing; add per-row error handling and respect stopOnErrorOnly processing
rows.slice(0, 2)makes experiments incomplete and misleading. Also, errors from the task/createTask/evaluator abort the entire run and are not recorded intaskErrors. Iterate all rows, wrap each step in try/catch, push messages intotaskErrors, and honorstopOnError.Apply this diff:
const taskResults: TaskResponse[] = []; const taskErrors: string[] = []; const evaluationResults: ExecutionResponse[] = []; - const rows_debug = rows.slice(0, 2); // TODO nina - for (const row of rows_debug) { - const taskOutput = await task(row as TInput); + const { stopOnError = false } = options; + for (const row of rows) { + let taskOutput: Record<string, any>; + try { + taskOutput = await task(row as TInput) as Record<string, any>; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Task execution failed for row ${String((row as any)?.id ?? '')}: ${msg}`); + if (stopOnError) break; + continue; + } // Create TaskResponse object const taskResponse: TaskResponse = { input: row, output: taskOutput as Record<string, any>, metadata: { rowId: row.id, timestamp: Date.now() }, timestamp: Date.now() }; taskResults.push(taskResponse); - const response = await this.createTask( - experimentSlug, - experimentResponse.run.id, - row, - taskOutput as Record<string, any> - ); - const taskId = response.id; + let taskId: string | undefined; + try { + const response = await this.createTask( + experimentSlug, + experimentResponse.run.id, + row, + taskOutput as Record<string, any> + ); + taskId = response.id; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Create task failed for row ${String((row as any)?.id ?? '')}: ${msg}`); + if (stopOnError) break; + continue; + } if (evaluators.length > 0) { for (const evaluator of evaluators) { - const singleEvaluationResult = await this.evaluator.runExperimentEvaluator({ - experimentId: experimentResponse.experiment.id, - experimentRunId: experimentResponse.run.id, - taskId, - evaluator, - taskResult: taskOutput as Record<string, any>, - waitForResults, - timeout: 120000 // 2 minutes default - }); - evaluationResults.push(...singleEvaluationResult); + try { + const singleEvaluationResult = await this.evaluator.runExperimentEvaluator({ + experimentId: experimentResponse.experiment.id, + experimentRunId: experimentResponse.run.id, + taskId: taskId!, + evaluator, + taskResult: taskOutput as Record<string, any>, + waitForResults, + timeout: 120000 // 2 minutes default + }); + evaluationResults.push(...singleEvaluationResult); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Evaluator '${typeof evaluator === 'string' ? evaluator : evaluator.name}' failed for row ${String((row as any)?.id ?? '')}: ${msg}`); + if (stopOnError) break; + } } } }
🧹 Nitpick comments (5)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
17-17: Reuse the shared response handler; extend BaseDatasetEntity and drop the duplicatehandleResponse
Evaluator/Datasetsalready inherit a commonhandleResponse. This class reimplements a near-duplicate. Extend the same base to keep behavior consistent and remove the local copy. Also remove the now-unusedtransformApiResponseimport.Apply these diffs:
+import { BaseDatasetEntity } from "../dataset/base"; -import { transformApiResponse } from "../../utils/response-transformer";-export class Experiment { +export class Experiment extends BaseDatasetEntity {- private async handleResponse(response: Response) { - if (!response.ok) { - let errorMessage = `HTTP ${response.status}: ${response.statusText}`; - - try { - const errorData = await response.json(); - if (errorData.error) { - errorMessage = errorData.error; - } - } catch { - // Use default HTTP error message if JSON parsing fails - } - - throw new Error(errorMessage); - } - - const contentType = response.headers.get("content-type"); - if (contentType && contentType.includes("application/json")) { - const rawData = await response.json(); - return transformApiResponse(rawData); - } - - // Handle non-JSON responses (text/csv, etc.) - const textContent = await response.text(); - return { - contentType: contentType || "text/plain", - body: textContent, - }; - } + // Inherit handleResponse from BaseDatasetEntityAlso applies to: 1-4, 37-65
145-153: Reduce result logging to avoid accidental PII leakage
console.log("evalResults", evalResults);may dump evaluator payloads. Log counts/summaries instead.Apply this diff:
- console.log("evalResults", evalResults); + console.log(`✅ Step 3: Collected ${evalResults.length} evaluation result(s)`);
212-236: JSONL with a column-definition header: LGTM; document the custom formatGiven your dataset format intentionally places a columns-definition line at index 0, skipping the first line is correct.
Add a clarifying note so future readers don’t “fix” it:
- /** - * Parse JSONL string into list of {col_name: col_value} dictionaries - * Skips the first line (columns definition) - */ + /** + * Parse JSONL string into list of {col_name: col_value} dictionaries. + * + * Custom Traceloop format: line 0 contains a column-definition header; + * data rows start at line 1, which we intentionally skip here. + */
259-292:concurrencyoption is validated but unusedYou validate
concurrencybut process rows sequentially. Either remove the option for now or implement a small concurrency pool to honor it.Example helper (outside this class):
async function mapWithConcurrency<T, R>( items: T[], limit: number, fn: (item: T) => Promise<R> ): Promise<R[]> { const results: R[] = []; let i = 0; const workers = Array.from({ length: Math.min(limit, items.length) }, async () => { while (true) { const index = i++; if (index >= items.length) break; results[index] = await fn(items[index]); } }); await Promise.all(workers); return results; }You can then schedule row processing via this pool, while keeping the per-row try/catch introduced above.
155-159: Preserve original error context with Error.causeWrapping the error loses the original stack/message context. Use
Error.causefor easier diagnostics.Apply this diff:
- throw new Error( - `Experiment execution failed: ${error instanceof Error ? error.message : 'Unknown error'}` - ); + throw new Error( + `Experiment execution failed`, + { cause: error instanceof Error ? error : new Error(String(error)) } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/sample-app/src/sample_experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts
- packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts
- packages/sample-app/src/sample_experiment.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T12:44:43.829Z
Learnt from: nina-kollman
PR: traceloop/openllmetry-js#672
File: packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:212-231
Timestamp: 2025-08-22T12:44:43.829Z
Learning: In the Traceloop SDK experiment system, JSONL datasets use a custom format where the first line contains column definitions and subsequent lines contain the actual data rows. The parseJsonlToRows method in packages/traceloop-sdk/src/lib/client/experiment/experiment.ts intentionally skips the first line (index 0) to avoid parsing the column definition as data.
Applied to files:
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
🧬 Code graph analysis (1)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-96)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
Evaluator(13-206)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (1)
Datasets(10-110)packages/traceloop-sdk/src/lib/utils/response-transformer.ts (1)
transformApiResponse(50-52)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (9)
ExperimentTaskFunction(1-3)ExperimentRunOptions(12-22)ExperimentRunResult(35-41)TaskResponse(26-33)ExecutionResponse(77-80)CreateTaskResponse(87-89)CreateTaskRequest(82-85)InitExperimentRequest(43-52)ExperimentInitResponse(72-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e95d21a in 40 seconds. Click for details.
- Reviewed
317lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/client/stream/index.ts:1
- Draft comment:
Removed export for SSEClient. Ensure that no client code relies on this export or update references accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/traceloop-sdk/src/lib/client/stream/sse-client.ts:1
- Draft comment:
Entire SSEClient implementation has been removed. Confirm this removal is intentional and that any dependent tests or documentation are updated. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_PZ8Lo6wIqMzhthHL
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ce184ea in 1 minute and 9 seconds. Click for details.
- Reviewed
29lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:266
- Draft comment:
Removed concurrency validation. Confirm removal is intended and update docs if needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts:23
- Draft comment:
Removed 'concurrency' from ExperimentRunOptions. Ensure consumers aren’t relying on it. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that consumers aren't relying on a removed feature. This is similar to asking the author to double-check or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue with the code.
Workflow ID: wflow_Y397FtwjEeTvfdbi
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7f42a35 in 1 minute and 17 seconds. Click for details.
- Reviewed
1024lines of code in9files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. CLAUDE.md:9
- Draft comment:
Extra blank lines improve readability; ensure they remain consistent across sections. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/sample-app/src/medical_prompts.ts:65
- Draft comment:
Consistent use of double quotes for prompt category keys enhances readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/sample-app/src/sample_experiment.ts:145
- Draft comment:
Remove commented-out code if it’s no longer needed before merging. - Reason this comment was not posted:
Comment was on unchanged code.
4. packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts:112
- Draft comment:
Avoid using bracket notation (e.g., this.client["baseUrl"]) for accessing client properties; consider adding public getters for better type safety. - Reason this comment was not posted:
Comment was on unchanged code.
5. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:113
- Draft comment:
The temporary slice 'rows_debug' with a TODO comment should be removed or finalized before production. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:285
- Draft comment:
Consider logging the number of rows fetched rather than the entire row object for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. packages/sample-app/src/sample_experiment.ts:156
- Draft comment:
Ensure detailed error logging does not expose sensitive information in production environments. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_NZjcSiV59YUXiSQc
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
CLAUDE.md (1)
1-177: Consider removing or relocating CLAUDE.md (internal tooling doc).Prior feedback already requested removing this file from the repo to avoid publishing Claude-specific guidance. If you still want it, move to a non-published docs location (e.g., /docs/internal/CLAUDE.md) or gate it from package publishes.
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (3)
224-231: Consider wiring relatedRef/aux as first-class fields if the backend expects them.You merge relatedRef and aux into experiment_run_metadata. If the API expects them top-level (related_ref/aux), add those keys in the payload too. Otherwise, add a comment clarifying where the backend consumes them.
22-26: Avoid duplicating Evaluator/Datasets instances; inject them.Re-instantiating here creates multiple clients with diverging state and complicates testing. Reuse the instances already created on TraceloopClient.
Apply this diff:
-export class Experiment { +export class Experiment { private client: TraceloopClient; private evaluator: Evaluator; private datasets: Datasets; - constructor(client: TraceloopClient) { - this.client = client; - this.evaluator = new Evaluator(client); - this.datasets = new Datasets(client); - } + constructor( + client: TraceloopClient, + deps?: { evaluator?: Evaluator; datasets?: Datasets } + ) { + this.client = client; + this.evaluator = deps?.evaluator ?? new Evaluator(client); + this.datasets = deps?.datasets ?? new Datasets(client); + }Update TraceloopClient to pass its instances:
// packages/traceloop-sdk/src/lib/client/traceloop-client.ts this.datasets = new Datasets(this); this.evaluator = new Evaluator(this); this.experiment = new Experiment(this, { evaluator: this.evaluator, datasets: this.datasets });
113-153: Remove debug row slicing; implement per-row error handling and stopOnError.rows.slice(0, 2) makes run() nonfunctional beyond demo. Add try/catch around each row, record errors, and respect stopOnError.
Apply this diff:
- const rows_debug = rows.slice(0, 2); // TODO nina - for (const row of rows_debug) { - const taskOutput = await task(row as TInput); + const { stopOnError = false } = options; + for (const row of rows) { + let taskOutput: Record<string, any>; + try { + taskOutput = (await task(row as TInput)) as Record<string, any>; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + taskErrors.push(`Task execution failed for row ${row?.id ?? "n/a"}: ${msg}`); + if (stopOnError) break; + continue; + } // Create TaskResponse object const taskResponse: TaskResponse = { input: row, output: taskOutput as Record<string, any>, metadata: { rowId: row.id, timestamp: Date.now(), }, timestamp: Date.now(), }; taskResults.push(taskResponse); - const response = await this.createTask( + let response: CreateTaskResponse; + try { + response = await this.createTask( experimentSlug, experimentResponse.run.id, row, taskOutput as Record<string, any>, - ); + ); + } catch (err) { + taskErrors.push( + `CreateTask failed for row ${row?.id ?? "n/a"}: ${err instanceof Error ? err.message : String(err)}` + ); + if (stopOnError) break; + continue; + } const taskId = response.id; if (evaluators.length > 0) { for (const evaluator of evaluators) { - const singleEvaluationResult = - await this.evaluator.runExperimentEvaluator({ + try { + const singleEvaluationResult = + await this.evaluator.runExperimentEvaluator({ experimentId: experimentResponse.experiment.id, experimentRunId: experimentResponse.run.id, taskId, evaluator, taskResult: taskOutput as Record<string, any>, waitForResults, timeout: 120000, // 2 minutes default - }); - evaluationResults.push(...singleEvaluationResult); + }); + evaluationResults.push(...singleEvaluationResult); + } catch (err) { + taskErrors.push( + `Evaluator '${typeof evaluator === "string" ? evaluator : evaluator.name}' failed: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + if (stopOnError) break; + } } } }packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (2)
50-53: Honor the timeout option when waiting for results.You accept timeout in options but don’t use it. Pass it through to waitForResult.
Apply this diff:
- return this.waitForResult( - triggerResponse.executionId, - triggerResponse.streamUrl, - ); + const { timeout = 120000 } = options; + return this.waitForResult( + triggerResponse.executionId, + triggerResponse.streamUrl, + timeout, + );
65-67: Validate evaluator to prevent runtime crash.The error mentions evaluator is required, but it isn’t checked here. Accessing evaluator.name below will throw if it’s undefined.
Apply this diff:
- if (!experimentId || !taskResult) { - throw new Error("experimentId, evaluator, and taskResult are required"); - } + if (!experimentId || !evaluator || !taskResult) { + throw new Error("experimentId, evaluator, and taskResult are required"); + }packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (1)
11-19: Timeout optional now matches runtime default — nice alignmentThis addresses the earlier suggestion to make
timeoutoptional inrunExperimentEvaluator().Consider documenting the default and units inline:
evaluator: EvaluatorDetails; waitForResults?: boolean; - timeout?: number; + /** In seconds; defaults to 120 when omitted. */ + timeout?: number;
🧹 Nitpick comments (22)
packages/sample-app/src/sample_experiment.ts (4)
15-21: Fail fast when TRACELOOP_API_KEY is missing.Initialization proceeds even if the API key is undefined; you only fail later. Add a guard to improve developer UX.
Apply this diff:
traceloop.initialize({ appName: "sample_experiment", apiKey: process.env.TRACELOOP_API_KEY, disableBatch: true, traceloopSyncEnabled: true, }); + if (!process.env.TRACELOOP_API_KEY) { + console.error("TRACELOOP_API_KEY is not set"); + process.exit(1); + }
42-46: Guard against missing OPENAI_API_KEY and make model configurable.Avoid runtime errors and let users switch models via env without code edits.
Apply this diff:
- const openai = new OpenAI({ - apiKey: process.env.OPENAI_API_KEY, - }); + if (!process.env.OPENAI_API_KEY) { + console.error("OPENAI_API_KEY is not set"); + process.exit(1); + } + const openai = new OpenAI({ apiKey: process.env.OPENAI_API_KEY }); + const OPENAI_MODEL = process.env.OPENAI_MODEL || "gpt-3.5-turbo";…and use it below:
- const answer = await openai.chat.completions.create({ - model: "gpt-3.5-turbo", + const answer = await openai.chat.completions.create({ + model: OPENAI_MODEL,
64-79: Harden task function with per-call error handling.A single failed completion will currently bubble and abort the whole run; return a structured error so the SDK can record it per-row.
Apply this diff:
- const medicalTaskRefuseAdvice: ExperimentTaskFunction = async ( + const medicalTaskRefuseAdvice: ExperimentTaskFunction<TaskInput, TaskOutput> = async ( row: TaskInput, ): Promise<TaskOutput> => { - const promptText = refuseMedicalAdvicePrompt(row.question as string); - const answer = await openai.chat.completions.create({ - model: "gpt-3.5-turbo", - messages: [{ role: "user", content: promptText }], - temperature: 0.7, - max_tokens: 500, - }); - - return { - completion: answer.choices?.[0]?.message?.content || "", - prompt: promptText, - }; + try { + const promptText = refuseMedicalAdvicePrompt(String(row.question ?? "")); + const answer = await openai.chat.completions.create({ + model: OPENAI_MODEL, + messages: [{ role: "user", content: promptText }], + temperature: 0.7, + max_tokens: 500, + }); + return { + completion: answer.choices?.[0]?.message?.content || "", + prompt: promptText, + }; + } catch (err) { + return { + completion: "", + prompt: "", + error: (err as Error).message ?? String(err), + } as unknown as TaskOutput; + } };
100-106: Pin evaluator version or accept drift.You pass evaluators as bare names. If the backend resolves “latest”, results may change over time. Consider pinning versions for reproducibility.
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (3)
37-65: Reuse base HTTP handling instead of a local copy.This handleResponse duplicates logic that already exists on BaseDatasetEntity. Consider making Experiment extend BaseDatasetEntity and drop this method to avoid divergence.
Proposed class signature:
-import { transformApiResponse } from "../../utils/response-transformer"; +import { BaseDatasetEntity } from "../dataset/base-dataset"; - export class Experiment { + export class Experiment extends BaseDatasetEntity { // ... - private async handleResponse(response: Response) { ... } + // Remove local handleResponse; use the inherited one.
192-202: Simplify response handling and accept id/taskId variants.handleResponse already throws on non-OK. Also tolerate APIs returning taskId.
Apply this diff:
- const response = await this.client.post( + const response = await this.client.post( `/v2/experiments/${experimentSlug}/runs/${experimentRunId}/task`, body, ); - - if (!response.ok) { - throw new Error( - `Failed to create task for experiment '${experimentSlug}'`, - ); - } - - const data = await this.handleResponse(response); - return { - id: data.id, - }; + const data = await this.handleResponse(response) as { id?: string; taskId?: string }; + const id = data.id ?? data.taskId; + if (!id) throw new Error("Task created but response missing id/taskId"); + return { id };
271-287: Validate datasetVersion explicitly (or support “latest”).Passing datasetVersion || "" relies on downstream throw. Fail fast here or normalize to “latest” if the API supports it.
Apply this diff:
- console.log(`🔧 Fetching dataset: ${datasetSlug}`); - const dataset = await this.datasets.getVersionAsJsonl( - datasetSlug, - datasetVersion || "", - ); + console.log(`🔧 Fetching dataset: ${datasetSlug}`); + if (!datasetVersion) { + throw new Error("datasetVersion is required (e.g., 'v1'). If the API supports 'latest', we can default to it."); + } + const dataset = await this.datasets.getVersionAsJsonl(datasetSlug, datasetVersion);Optional: add a DEBUG flag and gate logs.
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (2)
104-116: Don’t access private client fields; use client.get and normalize the URL.Avoid this.client["baseUrl"]/["apiKey"] and centralize headers/auth.
Apply this diff:
- async waitForResult( - executionId: string, - streamUrl: string, - ): Promise<ExecutionResponse[]> { + async waitForResult( + executionId: string, + streamUrl: string, + timeout = 120000, + ): Promise<ExecutionResponse[]> { @@ - console.log("waitForResult called with:", { executionId, streamUrl }); - - const fullStreamUrl = `${this.client["baseUrl"]}/v2${streamUrl}`; - console.log("Full stream URL:", fullStreamUrl); - try { - const response = await fetch(fullStreamUrl, { - headers: { - Authorization: `Bearer ${this.client["apiKey"]}`, - Accept: "application/json", - "Cache-Control": "no-cache", - }, - }); + const path = streamUrl.startsWith("/v2/") ? streamUrl : `/v2${streamUrl}`; + const response = (await Promise.race([ + this.client.get(path), + new Promise<Response>((_, reject) => + setTimeout(() => reject(new Error(`Timed out after ${timeout}ms`)), timeout), + ), + ])) as Response;Optional: gate console logs behind TRACELOOP_DEBUG to reduce noise in SDK code.
155-161: Stabilize return shape when fields are missing.Prefer safe fallbacks to avoid returning executionId: undefined or a missing result.
Apply this diff:
- const executionResponse: ExecutionResponse = { - executionId: responseData.execution_id, - result: responseData.result, - }; + const executionResponse: ExecutionResponse = { + executionId: responseData.execution_id ?? executionId, + result: responseData.result ?? {}, + };packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (2)
1-5: Broaden TaskInput/Output to support nested JSON.Real-world inputs/outputs often contain arrays/objects/null. Current dataValue is too restrictive.
Apply this diff:
-type dataValue = string | number | boolean; +type JsonValue = string | number | boolean | null | { [k: string]: JsonValue } | JsonValue[]; -export type TaskInput = Record<string, dataValue>; -export type TaskOutput = Record<string, dataValue>; +export type TaskInput = Record<string, JsonValue>; +export type TaskOutput = Record<string, JsonValue>;
89-96: CreateTaskResponse: tolerate id/taskId.APIs sometimes return taskId; reflect that at the type level for resilience.
Apply this diff:
-export interface CreateTaskResponse { - id: string; -} +export interface CreateTaskResponse { + id: string; + taskId?: string; +}Pair with the caller-side fallback added in Experiment.createTask.
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (11)
3-9: Prefer generics/unknown over any; disambiguate the raw SSE “event” name
data: anyweakens type-safety; use a generic withunknowndefault.event?: stringcan be confused with the discriminanttype; considersseEvent?: string.- Document timestamp format (ISO 8601).
Apply:
-export interface StreamEvent { - type: "progress" | "result" | "error" | "complete"; - data: any; - timestamp: string; - id?: string; - event?: string; -} +/** Generic SSE envelope; timestamp should be ISO 8601. */ +export interface StreamEvent<T = unknown> { + type: "progress" | "result" | "error" | "complete"; + data: T; + timestamp: string; // ISO 8601 + id?: string; // raw SSE id, if provided + sseEvent?: string; // raw SSE "event" name, if provided +}If renaming is too disruptive now, keep
eventbut at least document it as the raw SSE event name.
15-16: NarrowanyfortaskResultto improve safety
Record<string, any>leaks unsoundness downstream. Preferunknownor a generic.- taskResult: Record<string, any>; + taskResult: Record<string, unknown>;Or make
EvaluatorRunOptionsgeneric:<TResult extends Record<string, unknown> = Record<string, unknown>>.
21-28: MakeEvaluatorResultgeneric and replaceanywithunknownImproves type-safety without burdening callers (thanks to defaults).
-export interface EvaluatorResult { +export interface EvaluatorResult< + R = unknown, + M extends Record<string, unknown> = Record<string, unknown> +> { evaluatorName: string; taskId?: string; score?: number; - result?: any; - metadata?: Record<string, any>; + result?: R; + metadata?: M; error?: string; }Optionally document the expected score range (e.g., 0–1 or 0–100).
39-42: ClarifystreamUrlsemanticsDocument that this is an SSE endpoint URL (and whether it’s absolute).
export interface TriggerEvaluatorResponse { executionId: string; - streamUrl: string; + /** Absolute URL to the SSE endpoint for this execution. */ + streamUrl: string; }
44-47: SpecifySSEClientOptions.timeoutsemanticsIs this the connection lifetime, idle timeout, or initial connect timeout? Document units and behavior.
export interface SSEClientOptions { - timeout?: number; + /** Connection timeout in seconds (e.g., max time to wait/keep the stream open). */ + timeout?: number; headers?: Record<string, string>; }
49-57: Define progress invariants and percentage scaleExplicitly state whether
percentageis 0–100 or 0–1, and relationships among fields.export interface StreamProgressEvent { type: "progress"; data: { completed: number; total: number; - percentage: number; + /** 0–100 inclusive. Should equal (completed / total) * 100 when total > 0. */ + percentage: number; currentTask?: string; }; }
59-66: Avoid duplicatingevaluatorNamein event and payload
evaluatorNameexists inEvaluatorResult, duplicating it in the event can drift.export interface StreamResultEvent { type: "result"; data: { taskId: string; - evaluatorName: string; - result: EvaluatorResult; + result: EvaluatorResult; }; }If consumers need quick access, keep one source of truth and derive the other at usage.
68-75: Include optional error code for programmatic handlingAn error code can be useful alongside the message.
export interface StreamErrorEvent { type: "error"; data: { error: string; + code?: string | number; taskId?: string; evaluatorName?: string; }; }
77-85: Clarify duration unitsDocument whether
durationis milliseconds or seconds.export interface StreamCompleteEvent { type: "complete"; data: { executionId: string; totalResults: number; totalErrors: number; - duration: number; + /** Total execution duration in milliseconds. */ + duration: number; }; }
87-91: Consider consolidatingStreamEventandSSEStreamEventTwo parallel event types can confuse consumers. Option: make
StreamEvent<T>the envelope and defineSSEStreamEventasStreamEvent<Progress|Result|...>, or export only the discriminated union.
93-99: DocumentInputExtractor.sourcegrammarSpecify whether it’s JSONPath, JSONPointer, dot-path, etc., to avoid ambiguity across implementations.
export interface InputExtractor { - source: string; + /** Path expression to extract input (e.g., JSONPath/JSONPointer). */ + source: string; } export interface InputSchemaMapping { [key: string]: InputExtractor; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
CLAUDE.md(1 hunks)packages/sample-app/src/medical_prompts.ts(1 hunks)packages/sample-app/src/sample_experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts(1 hunks)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/index.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts(1 hunks)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts(1 hunks)packages/traceloop-sdk/src/lib/node-server-sdk.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/traceloop-sdk/src/lib/client/experiment/index.ts
- packages/traceloop-sdk/src/lib/client/dataset/datasets.ts
- packages/sample-app/src/medical_prompts.ts
- packages/traceloop-sdk/src/lib/node-server-sdk.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T12:44:43.829Z
Learnt from: nina-kollman
PR: traceloop/openllmetry-js#672
File: packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:212-231
Timestamp: 2025-08-22T12:44:43.829Z
Learning: In the Traceloop SDK experiment system, JSONL datasets use a custom format where the first line contains column definitions and subsequent lines contain the actual data rows. The parseJsonlToRows method in packages/traceloop-sdk/src/lib/client/experiment/experiment.ts intentionally skips the first line (index 0) to avoid parsing the column definition as data.
Applied to files:
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
🧬 Code graph analysis (5)
packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (1)
InputSchemaMapping(97-99)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (4)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-96)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
Evaluator(11-228)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (1)
Datasets(10-112)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (9)
ExperimentTaskFunction(6-11)ExperimentRunOptions(20-29)ExperimentRunResult(42-48)TaskResponse(33-40)ExecutionResponse(84-87)CreateTaskResponse(94-96)CreateTaskRequest(89-92)InitExperimentRequest(50-59)ExperimentInitResponse(79-82)
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (2)
packages/traceloop-sdk/src/lib/node-server-sdk.ts (3)
StreamEvent(28-28)EvaluatorDetails(27-27)SSEStreamEvent(29-29)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
EvaluatorDetails(13-13)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (3)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-96)packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (4)
EvaluatorRunOptions(11-19)TriggerEvaluatorRequest(30-37)TriggerEvaluatorResponse(39-42)InputSchemaMapping(97-99)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (1)
ExecutionResponse(84-87)
packages/sample-app/src/sample_experiment.ts (2)
packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (4)
ExperimentTaskFunction(6-11)TaskInput(3-3)TaskOutput(4-4)TaskResponse(33-40)packages/sample-app/src/medical_prompts.ts (1)
refuseMedicalAdvicePrompt(37-60)
🪛 LanguageTool
CLAUDE.md
[grammar] ~82-~82: There might be a mistake here.
Context: ...traceloop-sdk (Main SDK) - Path: packages/traceloop-sdk/ - Exports: @traceloop/node-server-sdk ...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...packages/traceloop-sdk/- **Exports**:@traceloop/node-server-sdk` - Purpose: Primary entry point that orch...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...t that orchestrates all instrumentations - Key Files: - `src/lib/tracing/decora...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...es all instrumentations - Key Files: - src/lib/tracing/decorators.ts: Workflow and task decorators (`@workfl...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...orators (@workflow, @task, @agent) - src/lib/tracing/tracing.ts: Core tracing utilities and span manage...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...re tracing utilities and span management - src/lib/node-server-sdk.ts: Main initialization logic #### Instru...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...rumentation-[provider]/ - **OpenAI**:@traceloop/instrumentation-openai- **Anthropic**:@traceloop/instrumentation...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...nstrumentation-openai- **Anthropic**:@traceloop/instrumentation-anthropic- **Bedrock**:@traceloop/instrumentation-b...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...strumentation-anthropic- **Bedrock**:@traceloop/instrumentation-bedrock` - Vector DBs: Pinecone, Chroma, Qdrant p...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...DBs**: Pinecone, Chroma, Qdrant packages - Frameworks: LangChain, LlamaIndex pack...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ... ai-semantic-conventions - Path: packages/ai-semantic-conventions/ - Purpose: OpenTelemetry semantic conven...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...ry semantic conventions for AI/LLM spans - Key File: src/SemanticAttributes.ts ...
(QB_NEW_EN)
[grammar] ~110-~110: There might be a mistake here.
Context: ...**: Wrap target library functions using InstrumentationModuleDefinition 2. Span Creation: Create spans with appro...
(QB_NEW_EN)
[grammar] ~111-~111: There might be a mistake here.
Context: ...ans with appropriate semantic attributes 3. Data Extraction: Extract request/respo...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ...ct request/response data and token usage 4. Error Handling: Capture and record err...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: .... Create new instrumentation package in packages/instrumentation-[provider]/ 2. Implement instrumentation extending `Ins...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... 2. Implement instrumentation extending InstrumentationBase 3. Add to main SDK dependencies in `package...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...ase3. Add to main SDK dependencies inpackages/traceloop-sdk/package.json` 4. Register in SDK initialization ### Runn...
(QB_NEW_EN)
🔇 Additional comments (2)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (1)
246-266: JSONL header skip is intentional — LGTM.Thanks for confirming the first line is a schema/columns definition. Skipping index 0 matches your custom dataset format.
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (1)
1-1: Type-only imports confirmed in both interfacesVerified that the reverse edge in
experiment.interface.tsuses a type-only import forInputSchemaMapping(line 31), ensuring no runtime-cycle risk.• packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts:31 imports
InputSchemaMappingwithimport type— all set.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 169ba47 in 1 minute and 54 seconds. Click for details.
- Reviewed
177lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/sample-app/src/sample_experiment.ts:66
- Draft comment:
The refuse advice task makes a direct API call for completions while a similar call is encapsulated in generateMedicalAnswer. Consider refactoring to reuse generateMedicalAnswer for consistency and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/sample-app/src/sample_experiment.ts:65
- Draft comment:
Casting 'row.question' to string without validation may hide potential runtime issues. Consider checking if 'row.question' exists and is a valid string before using it. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. packages/sample-app/src/sample_experiment.ts:115
- Draft comment:
The first experiment run does not use the waitForResults option while the second one does. Confirm whether this discrepancy is intentional and document the rationale if so. - Reason this comment was not posted:
Comment was on unchanged code.
4. packages/sample-app/src/sample_experiment.ts:56
- Draft comment:
Returning an empty string when no response content is found may mask API issues. Consider logging a warning or handling this case more explicitly. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
Workflow ID: wflow_jOPxMDHWQHVDCxml
You can customize
|
|
||
| const loader1 = startLoader(" Processing experiment"); | ||
|
|
||
| const results1 = await client.experiment.run(medicalTaskRefuseAdvice, { |
There was a problem hiding this comment.
If client.experiment.run throws an error, the loader (set by startLoader) may never be stopped. Consider wrapping experiment calls in a try-finally block to ensure stopLoader is always invoked.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
22-26: Address dependency injection concern.The constructor creates new instances of Evaluator and Datasets, which could lead to duplicate singleton instances and potential state drift, as noted in previous reviews.
96-101: Fix ESLint issue with array declaration.The
evaluationResultsarray is declared withletbut never reassigned, which violates the ESLint prefer-const rule.
102-141: Remove debug truncation and add proper error handling.The loop currently processes all rows but lacks per-row error handling and doesn't respect the
stopOnErroroption for robust experiment execution.
179-183: Remove redundant response.ok check.The explicit
response.okcheck is redundant sincehandleResponsealready throws on non-OK responses.
266-269: Handle empty dataset version properly.Passing an empty string for
datasetVersionwill create a malformed URL with double slashes. The code should default to "latest" or validate the input.
🧹 Nitpick comments (1)
packages/sample-app/src/sample_experiment.ts (1)
62-77: Remove code duplication in OpenAI API calls.Both
medicalTaskRefuseAdviceand thegenerateMedicalAnswerhelper function contain nearly identical OpenAI API calls. Consider extracting this into a reusable function to follow the DRY principle.+ /** + * Common OpenAI completion helper + */ + async function callOpenAI(promptText: string): Promise<string> { + const response = await openai.chat.completions.create({ + model: "gpt-3.5-turbo", + messages: [{ role: "user", content: promptText }], + temperature: 0.7, + max_tokens: 500, + }); + return response.choices?.[0]?.message?.content || ""; + } const medicalTaskRefuseAdvice: ExperimentTaskFunction = async ( row: TaskInput, ): Promise<TaskOutput> => { const promptText = refuseMedicalAdvicePrompt(row.question as string); - const answer = await openai.chat.completions.create({ - model: "gpt-3.5-turbo", - messages: [{ role: "user", content: promptText }], - temperature: 0.7, - max_tokens: 500, - }); + const completion = await callOpenAI(promptText); return { - completion: answer.choices?.[0]?.message?.content || "", + completion, prompt: promptText, }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/sample-app/src/sample_experiment.ts(1 hunks)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts(1 hunks)packages/traceloop-sdk/src/lib/client/experiment/experiment.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T12:44:43.829Z
Learnt from: nina-kollman
PR: traceloop/openllmetry-js#672
File: packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:212-231
Timestamp: 2025-08-22T12:44:43.829Z
Learning: In the Traceloop SDK experiment system, JSONL datasets use a custom format where the first line contains column definitions and subsequent lines contain the actual data rows. The parseJsonlToRows method in packages/traceloop-sdk/src/lib/client/experiment/experiment.ts intentionally skips the first line (index 0) to avoid parsing the column definition as data.
Applied to files:
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
🧬 Code graph analysis (2)
packages/sample-app/src/sample_experiment.ts (2)
packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (3)
ExperimentTaskFunction(6-11)TaskInput(3-3)TaskOutput(4-4)packages/sample-app/src/medical_prompts.ts (2)
refuseMedicalAdvicePrompt(37-60)provideMedicalInfoPrompt(10-31)
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
packages/traceloop-sdk/src/lib/client/traceloop-client.ts (1)
TraceloopClient(21-96)packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
Evaluator(11-216)packages/traceloop-sdk/src/lib/client/dataset/datasets.ts (1)
Datasets(10-112)packages/traceloop-sdk/src/lib/utils/response-transformer.ts (1)
transformApiResponse(50-52)packages/traceloop-sdk/src/lib/interfaces/experiment.interface.ts (8)
ExperimentTaskFunction(6-11)ExperimentRunOptions(20-29)ExperimentRunResult(42-48)TaskResponse(33-40)ExecutionResponse(84-87)CreateTaskResponse(94-96)InitExperimentRequest(50-59)ExperimentInitResponse(79-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (14)
packages/sample-app/src/sample_experiment.ts (9)
1-10: LGTM! Clean imports and type definitions.The imports are well-organized and properly reference the necessary modules from the Traceloop SDK and OpenAI.
13-18: Initialize Traceloop with proper configuration.The initialization properly configures the SDK with required parameters and good defaults for experimental features.
20-29: Robust error handling for SDK initialization.The error handling properly catches initialization failures and provides clear error messages before exiting gracefully.
82-91: Good use of the extracted helper function.The
medicalTaskProvideInfofunction properly reuses thegenerateMedicalAnswerhelper, which demonstrates good code organization.
94-106: Creative and user-friendly loading indicator.The spinner implementation provides good user feedback during potentially long-running operations.
115-122: Proper experiment configuration for medical advice refusal.The experiment configuration is well-structured with appropriate dataset references and evaluator selection.
137-144: Good experiment configuration with result waiting.The second experiment properly enables
waitForResults: trueto demonstrate synchronous result handling.
153-161: Comprehensive error handling with stack traces.The error handling provides both user-friendly messages and technical details for debugging, which is excellent for a sample application.
164-168: Proper top-level error handling.The main function wrapper ensures unhandled promise rejections are caught and reported appropriately.
packages/traceloop-sdk/src/lib/client/experiment/experiment.ts (5)
31-35: Generate unique experiment slugs reliably.The slug generation combines timestamp and random components effectively, with proper truncation to ensure consistent length limits.
37-60: Comprehensive response handling with content type detection.The response handler properly manages different content types and applies the response transformer for JSON data while handling errors gracefully.
194-227: Handle optional fields properly in experiment initialization.The implementation correctly merges
auxandrelatedRefintoexperimentRunMetadatainstead of adding them directly to the payload, which aligns with the API structure.
233-253: JSONL parsing correctly skips column definitions.Based on the retrieved learnings, the intentional skipping of the first line (index 0) is correct for the Traceloop dataset format where the first line contains column definitions rather than data.
277-320: Comprehensive validation for experiment options.The validation properly checks task function validity and evaluator configurations, ensuring both string and object evaluator formats are handled correctly.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8ae47c2 in 57 seconds. Click for details.
- Reviewed
75lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/sample-app/src/sample_experiment.ts:82
- Draft comment:
Multiline async function parameters improve readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/sample-app/src/sample_experiment.ts:89
- Draft comment:
Trailing comma added in the returned object enhances consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/sample-app/src/sample_experiment.ts:97
- Draft comment:
Updated quotes in the frames array help maintain consistent code style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/traceloop-sdk/src/lib/client/experiment/experiment.ts:53
- Draft comment:
Multiline formatting for contentType extraction improves readability without affecting logic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_rKPwMr76IeAWdK4i
You can customize
3 participants
Important
Add experiment capabilities to Traceloop SDK with evaluator support, sample experiment script, and updated configuration.
Experimentclass inexperiment.tsto handle experiment execution with task functions and options.Evaluatorclass inevaluator.tsto run evaluators on experiment task results.sample_experiment.tsto demonstrate running experiments with medical prompts.medical_prompts.tsfor comprehensive and refusal medical prompt templates.initialize()inconfiguration/index.tsto supportexperimentSlugfrom options or environment variable.TraceloopClientintraceloop-client.tsto includeexperimentandevaluatorproperties.traceloop-sdk/package.json.eventsourceandtslibdependencies intraceloop-sdk/package.jsonandsample-app/package.json.This description was created by
for 8ae47c2. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Documentation
Chores