-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(deno): Add OpenTelemetry support and vercelAI integration #17445
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
Conversation
Thanks for the PR Sergiy! Just for me to understand this better: doesn't Deno come with a built-in otel tracer already? |
From chatting with @AbhiPrasad, we can't use it due to the limitations
|
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.
LGTM!
Looks like we can't use the deno tracer due to missing configuration APIs + v1 OTEL support only.
Thanks for adding this @sergical
* The only difference is that it does not use `@opentelemetry/instrumentation` | ||
* because Deno Workers do not support it in the same way. | ||
* | ||
* Therefore, we cannot automatically patch setting `experimental_telemetry: { isEnabled: true }` | ||
* and users have to manually set this to get spans. |
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.
I think this comment is wrong, I would just remove it - this is just a 1-1 copy of the cloudflare one, right? :D
So either we just say "this is a copt of the cloudflare one" or we copy the whole comment and say "it is a copy of the node one, but...."
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.
removed the comment, just left as copy from cloudflare
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.
generally this looks good to me - just to double check, does this conflict in any way with deno native otel support? We should make sure to test this that everything works as expected for both ends, so native otel stuff continues to work and our stuff continues to work if that is used 🤔
@mydea added tests for providers co-existing and not overriding each other, haven't tested end to end with actual OTeL instrumentation locally, lmk if this is enough or I should dig deeper |
Based on the Cloudflare OTel implementation