Feat: Opentelemetry for TS SDK#2828
Conversation
|
@jishnundth is attempting to deploy a commit to the Hatchet Team on Vercel. A member of the Team first needs to authorize it. |
5f7efa9 to
bf2bb93
Compare
|
Hey @mrkaye97, Please take a look at this, when you have time. The current approach follows monkey patching files for instrumentation during load time, which is the standard pattern used in OTel TypeScript examples as well. (References)
However, the issue with this approach is that register instrumentation must occur first, before loading any other libraries. This is because Node.js caches loaded libraries. If a library that needs to be instrumented is imported before the instrumentation is registered, the patching will not occur. The rest of the program will then use the cached version of the library, which does not contain the patched OTel code. As a result, the spans internally added in the patched module will not appear in HyperDX. You might already know this, just want to give a heads up on how opentelemetry works for JS. NOTE: Also, for testing with the examples, the code has to built, packed and installed in another I also had to edit |
|
Great, thanks! Will take a look soon :) |
|
btw just confirming - you've tested propagating the span context 1) from trigger -> run (so the trigger workflow run and the run workflow are part of the same trace, with the run nested as a span under the trigger) and 2) from parent -> child (when a task spawns a child the child should be nested under the parent)? |
Thanks for the message. Case 1 is properly tested. -> run is propagating correctly and showing proper nesting in the trace. |
|
@mrkaye97 Both cases work seemlessly. Please check the screenshots below Run WorkflowThis is the example in the PR. Where the workflow contains 4 tasks. 2 autoinstrumented ones (no separate spans), and 2 tasks having their own spans in the task.
Parent Child WorkflowA parent task spawing 3 child tasks. parent-orchestartion and spawn-child-int are spans inside the parent task.
Let me know, if there's issues. Also when you're testing please remember the note I shared above: #2828 (comment) |
Amazing! |
|
Hey @jishnundth, sorry this is taking so long. We just merged some major changes in |
1ca6aa4 to
44a3b52
Compare
No worries. I'm working on fixing things with the latest changes. Will keep you posted |
|
@mnafees please check now |
grutt
left a comment
There was a problem hiding this comment.
super close! a few questions and then we'll need to resolve conficts/lint
7d3722b to
255a90f
Compare
|
PTAL, the requested changes are added |
grutt
left a comment
There was a problem hiding this comment.
one last comment to add comments on core sdk functions to prevent future breakage
Done! |
|
📝 Documentation updates detected! Updated existing suggestion: Document OpenTelemetry support for Python, TypeScript, and Go SDKs Tip: Configure how Promptless handles changelogs in Agent Settings 📋 |
* add: otel as optional dep on ts packages * feat: opentelemetry instrumentor for TS sdk, with example * fix: lint * revert: debug print * remove: trailing space * fix: ts otel patch file path, throw handlesteprun error upstream, ts otel examples * fix: lint * feat: add schedule_workflow instrumentor, add otel conig loader tests * add: more robust wrap unwrap for patched modules * fix: lint, update version * refactor: ts otel config type assertion * revert: rebase issues * fix: lint * fix: update worker patch for ts otel with InternalWorker * fix: lint * refactor: parsejson on otel * fix: pnpm-lock * fix: lint * docs: add otel instrumented method warnings Co-authored-by: Jishnu <jishnun789@gmail.com>





Description
Type of change