-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
docs(bun): Add ESM/CJS and Bun-specific OTel docs #14594
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 javascript.mdx
files are just copied from the general docs page.
I only added some missing imports (BatchSpanProcessor
and OTLPTraceExporter
) and added the ESM version of it.
Bundle ReportChanges will increase total bundle size by 4.19kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-client-array-pushAssets Changed:
view changes for bundle: sentry-docs-server-cjsAssets Changed:
|
// Also filter out the BunServer integration to avoid emitting duplicated spans from Sentry AND your custom OTel instrumentation | ||
.filter((i) => i.name !== "Http" && i.name !== "BunServer") | ||
.concat(Sentry.httpIntegration({ spans: false })), |
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.
q: I thought we just have to filter out the bun integration, isn't spans: false
already covered by skipOpenTelemetrySetup: true
?
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 wanted it to align with the already existing docs (here) and they mention setting this option as well.
I'm not sure myself either how we should handle documenting this. I think it's fine writing it like this explicitly as it also supports older versions, where this was not yet the default.
@Lms24 @mydea what do you think of this here? This covers both the Node and Bun examples.
// Filter out the BunServer integration to avoid emitting spans from there | ||
.filter((i) => i.name !== "Http" && i.name !== "BunServer") | ||
// Add the httpIntegration again and disable emitting spans | ||
.concat(Sentry.httpIntegration({ spans: false })) |
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.
Same here.
// This leads to tracing being disabled | ||
|
||
// Disable emitting of spans in the httpIntegration | ||
integrations: [Sentry.httpIntegration({ spans: false })], |
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.
Same here.
// Disable emitting of spans in the httpIntegration | ||
integrations: [Sentry.httpIntegration({ spans: false })], |
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.
Same here.
@@ -41,6 +41,8 @@ Sentry.validateOpenTelemetrySetup(); | |||
|
|||
```javascript {tabTitle: NodeSDK} | |||
const Sentry = require("@sentry/node"); | |||
|
|||
const { NodeSDK } = require("@opentelemetry/sdk-node") |
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.
This is true, it's already imported on #L48
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.
Thanks!
DESCRIBE YOUR PR
This adds OTel documentation for bun. First, I added CJS/ESM snippets for all snippets to make it more inclusive.
For Bun, the biggest difference are the imports (
@sentry/bun
) and this snippet:closes getsentry/sentry-javascript#16969
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes: