-
Notifications
You must be signed in to change notification settings - Fork 243
Add improved OpenTelemetry instrumentation for workflow observability #933
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8ab0bd6
Add improved OpenTelemetry instrumentation for workflow observability
pranaygp 1e67403
Split changeset into separate entries for core and world-vercel
pranaygp 178fb1f
Update OTEL instrumentation to use standard semantic conventions
pranaygp 657ac4c
Address PR review comments
pranaygp b142e22
Use lowercase span names per OTEL semantic conventions
pranaygp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@workflow/core": patch | ||
| --- | ||
|
|
||
| Add OTEL tracing for event loading and queue timing breakdown using standard OTEL semantic conventions |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@workflow/world-vercel": patch | ||
| --- | ||
|
|
||
| Add OTEL tracing for HTTP requests and storage operations using standard OTEL semantic conventions |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * Utility to instrument object methods with tracing. | ||
| * This is a minimal version for world-vercel to avoid circular dependencies with @workflow/core. | ||
| */ | ||
| import { trace } from './telemetry.js'; | ||
|
|
||
| /** | ||
| * Wraps all methods of an object with tracing spans. | ||
| * @param prefix - Prefix for span names (e.g., "WORLD.runs") | ||
| * @param o - Object with methods to instrument | ||
| * @returns Instrumented object with same interface | ||
| */ | ||
| export function instrumentObject<T extends object>(prefix: string, o: T): T { | ||
| const handlers = {} as T; | ||
| for (const key of Object.keys(o) as (keyof T)[]) { | ||
| if (typeof o[key] !== 'function') { | ||
| handlers[key] = o[key]; | ||
| } else { | ||
| const f = o[key]; | ||
| // @ts-expect-error | ||
| handlers[key] = async (...args: any[]) => | ||
| trace(`${prefix}.${String(key)}`, {}, () => f(...args)); | ||
| } | ||
| } | ||
| return handlers; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The deserializeTimeMs measurement (line 243) captures only the synchronous portion of hydrateStepArguments. However, hydrateStepArguments may initiate async operations that are added to the 'ops' array (line 237), and these operations are executed later via Promise.all(ops) at line 294. This means the deserialize timing doesn't fully capture all deserialization work. Consider documenting this limitation or restructuring to measure the complete deserialization time including async operations.
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.
Added a comment documenting this limitation. The timing intentionally captures only the synchronous hydration - async operations like stream loading happen in background and their timing is included in the overall step execution time tracked separately.
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.
Maybe we also need separate spans explicitly for serializing and deserializing etc. rather than just including it as an attribute