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

Limit focused events to session-captured trace IDs #217

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

dcramer
Copy link
Member

@dcramer dcramer commented Dec 4, 2023

Adjust the behavior of Traces and Errors to show events which are associated with a known-local trace ID. If no trace IDs are known, assume all events should be present instead.

Fixes #188

image

Adjust the behavior of Traces and Errors to show events which are associated with a known-local trace ID. If no trace IDs are known, assume all events should be present instead.
Copy link

vercel bot commented Dec 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
spotlightjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 4, 2023 7:40pm

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Attention: 96 lines in your changes are missing coverage. Please review.

Comparison is base (c538cd0) 32.47% compared to head (97fe540) 32.21%.
Report is 1 commits behind head on main.

Files Patch % Lines
...y/src/integrations/sentry/components/EventList.tsx 16.12% 26 Missing ⚠️
...y/src/integrations/sentry/components/TraceList.tsx 20.00% 20 Missing ⚠️
...tegrations/sentry/components/HiddenItemsButton.tsx 10.00% 18 Missing ⚠️
.../src/integrations/sentry/data/useSentryHelpers.tsx 38.46% 8 Missing ⚠️
packages/overlay/src/integrations/sentry/index.ts 0.00% 8 Missing ⚠️
...ay/src/integrations/sentry/data/sentryDataCache.ts 75.00% 6 Missing ⚠️
packages/overlay/src/components/Badge.tsx 28.57% 5 Missing ⚠️
...rlay/src/integrations/sentry/sentry-integration.ts 16.66% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
- Coverage   32.47%   32.21%   -0.27%     
==========================================
  Files          39       42       +3     
  Lines        1869     1993     +124     
  Branches       71       71              
==========================================
+ Hits          607      642      +35     
- Misses       1262     1351      +89     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dcramer
Copy link
Member Author

dcramer commented Dec 4, 2023

One note - Sentry's Remix integration, and there's probably more, is currently not able to handle SSR errors.

That is, if you load a route, and it hits the top level ErrorBoundary, we fail to propagate the trace headers correctly. That means in this regard the error is shown as "in another session" because we've got an active frontend trace. However, that frontend trace is also detached from the backend hydration because of that same issue.

@dcramer
Copy link
Member Author

dcramer commented Dec 4, 2023

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.

I generally really like this change! We should be aware that the limitation you mentioned for Remix also exists for Astro SSR error pages (e.g. astro playground /ssr-error page) but maybe that's also okay. In fact, this might just trigger the fallback logic to show all events.

@Lms24
Copy link
Member

Lms24 commented Dec 5, 2023

I'm gonna wait with merging #228 until this is merged (if we still wanna merge before tomorrow).

@dcramer
Copy link
Member Author

dcramer commented Dec 5, 2023

Will follow up with a GH issue tracking this + an experimental flag for it. The Astro SSR case - at least in our playground - bypasses all session local logic so in that case the behavior is ok.

@dcramer dcramer merged commit 66f31fa into main Dec 5, 2023
4 of 6 checks passed
@dcramer dcramer deleted the feat/expose-local-events branch December 5, 2023 16:59
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.

Isolate Sentry Events in Spotlight to current tab
2 participants