Skip to content
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

feat(opentelemetry): Do not capture exceptions for timed events #11221

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

lforst
Copy link
Member

@lforst lforst commented Mar 21, 2024

We currently have logic in place that takes TimedEvents on OTEL spans and creates error events for them.

This creates a BIG problem when we do not control the instrumentationl, potentially creating a crap-ton of error events we likely do not even want. Exhibit a being Next.js creating TimedEvents for returning 404s and failed fetch requests (no matter if handled or not).

For the reason above, I would like to remove this logic completely for now, and we can add it back at a later point in time, maybe in the form of an (opt-in) integration with addtional options.

Ref #11016
Ref #11042

@lforst lforst mentioned this pull request Mar 21, 2024
13 tasks
@lforst lforst requested review from Lms24 and AbhiPrasad March 21, 2024 09:32
Copy link
Contributor

github-actions bot commented Mar 21, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) 80.73 KB (added)
@sentry/browser (incl. Tracing, Replay) 72.08 KB (added)
@sentry/browser (incl. Tracing, Replay with Canvas) 75.88 KB (added)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 65.64 KB (added)
@sentry/browser (incl. Tracing) 36.68 KB (added)
@sentry/browser (incl. browserTracingIntegration) 36.68 KB (added)
@sentry/browser (incl. feedbackIntegration) 31.35 KB (added)
@sentry/browser (incl. feedbackModalIntegration) 31.47 KB (added)
@sentry/browser (incl. feedbackScreenshotIntegration) 31.48 KB (added)
@sentry/browser (incl. sendFeedback) 27.43 KB (added)
@sentry/browser 22.6 KB (added)
CDN Bundle (incl. Tracing, Replay, Feedback) 75.07 KB (added)
CDN Bundle (incl. Tracing, Replay) 69.92 KB (added)
CDN Bundle (incl. Tracing) 36.27 KB (added)
CDN Bundle 23.97 KB (added)
CDN Bundle (incl. Tracing, Replay) - uncompressed 219.58 KB (added)
CDN Bundle (incl. Tracing) - uncompressed 109.53 KB (added)
CDN Bundle - uncompressed 71 KB (added)
@sentry/react (incl. Tracing, Replay) 72.07 KB (added)
@sentry/react 22.63 KB (added)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove this functionality, we're also stopping Otel SDK users (e.g. in some Node app) from capturing errors in Sentry in an Otel-native way, right? If this is correct, I think we want to avoid removing the functionality all together. Full disclosure: I wasn't even aware that this is currently possible so maybe I'm missing something.

Is there a way we could disable it for NextJS but keep it in lower level SDKs?

@lforst
Copy link
Member Author

lforst commented Mar 21, 2024

If we remove this functionality, we're also stopping Otel SDK users (e.g. in some Node app) from capturing errors in Sentry in an Otel-native way, right? If this is correct, I think we want to avoid removing the functionality all together. Full disclosure: I wasn't even aware that this is currently possible so maybe I'm missing something.

I would argue that no-one wants to do this at this point in time. I don't even think this is fully standardized yet and the otel folks are just figuring this out themselves. I think @bitsandfoxes was in convos with them. I tried to look for an Opentelemetry API to create TimedEvents that would fit into our error-creating scheme but couldn't find one so I don't think there is an "otel-native" way to do this yet. Me personally, I would not expect open telemetry to suddenly create more error events for me and the behavior that Next.js is doing would be very confusing in the product. Other instrumentation is bound to run into the same issue.

To me this feature is scope creep and I would vote we just don't do this now. As I mentioned in the PR description we can always make this opt-in in the future, or even default to it if we decide to. Also to be a bit populist: It's POTEL not EOTEL we are after right now. Step by step.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving to unblock, let's revisit this before we do a beta

@lforst lforst merged commit e878dce into develop Mar 21, 2024
58 checks passed
@lforst lforst deleted the lforst-no-error-events-for-timed-events branch March 21, 2024 16:47
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants