-
Notifications
You must be signed in to change notification settings - Fork 52
braintrust/temporal #1261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
braintrust/temporal #1261
Conversation
Qard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much we care about the peer dependency, but otherwise LGTM.
js/package.json
Outdated
| "@temporalio/workflow": "^1.11.0", | ||
| "zod": "^3.25.34 || ^4.0" | ||
| }, | ||
| "peerDependenciesMeta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yarn does not support peerDependenciesMeta. It will only yell at you about unsatisfied peer dependencies though but will continue installation anyway. Slight DX issue, but still works.
|
Does the peer dependency mean that you will have to install temporal when you install our SDK (although it is not bundled)? |
|
Because it has The DX sucks a bit as it will look like it is expecting temporal to be installed when it actually doesn't need it. But users can safely ignore that warning. |
ibolmo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to stop my review since I did find a non-starter.
| "include": ["otel_example.ts"], | ||
| "exclude": ["node_modules"] | ||
| } | ||
| "compilerOptions": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#nit i wonder why your setup did this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it prettier that did this?
js/src/wrappers/temporal/index.ts
Outdated
| * import { Client, Connection } from "@temporalio/client"; | ||
| * import { Worker } from "@temporalio/worker"; | ||
| * import * as braintrust from "braintrust"; | ||
| * import { createBraintrustTemporalPlugin } from "braintrust/wrappers/temporal"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#nit example needs an update to use braintrust.
| @@ -0,0 +1,128 @@ | |||
| import type { ClientPlugin, ClientOptions } from "@temporalio/client"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types are stripped off automatically when bundled, so this is ok still.
| ActivityInterceptors, | ||
| } from "@temporalio/worker"; | ||
| import type { WorkflowClientInterceptor } from "@temporalio/client"; | ||
| import { defaultPayloadConverter } from "@temporalio/common"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok this is not a type and will break braintrust installs that do not tree shake and that they don't have the temporalio client installed.
@Qard may confirm my understanding
looks like sanity tests are indeed failing (correctly): https://github.com/braintrustdata/braintrust-sdk/actions/runs/20974131523/job/60285471728?pr=1261#step:8:47
I think your only recourse is to create a separate package 🙉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Yes. Bundling woes. 🙈
This will indeed fail. Though the peer dependency thing is fine (ish), having imports of these anywhere that gets included in the bundle will indeed make it required as a proper dependency.
|
Hey there! I'm James, from the Temporal's SDK team. I'm not familiar with Is there any reason not to define the Temporal integration as a distinct package? This would ensure that your main package isn't linked to the Temporal SDK. Or am I missing something? |
- Add temporal wrapper with client, activity, and workflow interceptors - Export createBraintrustClientInterceptor, createBraintrustActivityInterceptor, createBraintrustSinks from braintrust package - Export workflow-interceptors as separate entry point for workflow isolate - Update example to use SDK exports instead of local implementation - Add mise kill task for force stopping server Co-Authored-By: Claude Opus 4.5 <[email protected]>
Activities running on different workers than the workflow now correctly parent to the workflow span instead of the client span. This is done by passing the workflowSpanId in headers and reconstructing the parent SpanComponentsV3 in the activity interceptor. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Tests for: - Header serialization/deserialization utilities - SpanComponentsV3 reconstruction for cross-worker parent propagation Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace manual interceptor setup with a simpler plugin API:
- Add BraintrustTemporalPlugin class implementing ClientPlugin and WorkerPlugin
- Add createBraintrustTemporalPlugin() factory function
- Update example to use plugin pattern
- Make interceptors internal (not exported from public API)
- Add plugin tests
Usage:
```typescript
const plugin = createBraintrustTemporalPlugin();
const client = new Client({ plugins: [plugin] });
const worker = await Worker.create({ plugins: [plugin], ... });
```
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Demonstrates that tracing context is properly propagated within activities, allowing nested spans to be created using braintrust.traced(). Co-Authored-By: Claude Opus 4.5 <[email protected]>
The test was incorrectly flagging @temporalio/client because "client" contains "cli". Changed the check to only match actual /cli/ directory paths, not package names that happen to contain "cli" as a substring. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…flowModules - Use `new BraintrustTemporalPlugin()` instead of factory function - Plugin now automatically adds workflow interceptors via workflowModules - Users no longer need to manually specify workflowModules in worker config Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Replace global variables with per-workflow state object - Each workflow gets its own isolated state via the factory function - Remove examples from pnpm workspace - they use published npm package Co-Authored-By: Claude Opus 4.5 <[email protected]>
d562403 to
035c055
Compare
e87d531 to
3b15fe9
Compare
clutchski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove mise? it makes things very simple (mise install to get all the tools, etc).

This PR adds braintrust tracing to temporal.
A new package @braintrust/temporal has been added that handles tracing temporal workflows.