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(overlay): Allow to directly add sentry envelopes via trigger #498

Closed
wants to merge 5 commits into from

Conversation

mydea
Copy link
Member

@mydea mydea commented Aug 20, 2024

I am currently working on a hackweek project where I want to integrate the spotlight overlay into a chrome browser extension.

There, I cannot use sidecar (because I cannot launch a node service), but I can intercept envelopes sent to sentry myself. I'd like to be able to just run the spotlight overlay in the extension and directly push envelopes to it.

For this, I added a new hook, so you can add envelopes like this:

trigger("sentry:addEnvelope", sentryEnvelope);

@mydea mydea requested review from BYK and Lms24 August 20, 2024 15:27
@mydea mydea self-assigned this Aug 20, 2024
Copy link

vercel bot commented Aug 20, 2024

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 Aug 27, 2024 10:06am

@BYK
Copy link
Member

BYK commented Aug 20, 2024

Nice, this looks something like #133 I guess.

Comment on lines 53 to 56
if (!e.detail.envelope) return;
processEnvelope({
contentType: HEADER,
data: e.detail.envelope,
Copy link
Member

Choose a reason for hiding this comment

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

Curious, why not just e.detail and pass the envelope as the event payload?

Copy link
Member Author

Choose a reason for hiding this comment

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

I basically just copied this from the showError scenario - but actually you are right, I think this is incorrect, will adjust it!

log('Sentry Event', e.detail.event_id);
if (!e.detail.event) return;
sentryDataCache.pushEvent(e.detail.event).then(() => open(`/errors/${e.detail.event.event_id}`));
const onRenderError = (e: CustomEvent<{ event?: SentryEvent; event_id: string }>) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

While at it I also typed this, for clarity - I think this is correct 😅 cc @BYK

@@ -155,7 +155,7 @@ import { trigger } from '@spotlightjs/spotlight';

trigger('sentry:showError', {
event: string,
eventId: string,
event_id: string,
Copy link
Member Author

Choose a reason for hiding this comment

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

I think these docs were incorrect? We use event_id in the code, as far as I can see 😅

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, sorry :(

mydea pushed a commit that referenced this pull request Aug 21, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and publish to npm
yourself or [setup this action to publish
automatically](https://github.com/changesets/action#with-publishing). If
you're not ready to do a release yet, that's fine, whenever you add more
changesets to fn/prerelease-addEnvelope, this PR will be updated.

⚠️⚠️⚠️⚠️⚠️⚠️

`fn/prerelease-addEnvelope` is currently in **pre mode** so this branch
has prereleases rather than normal releases. If you want to exit
prereleases, run `changeset pre exit` on `fn/prerelease-addEnvelope`.

⚠️⚠️⚠️⚠️⚠️⚠️

# Releases
## @spotlightjs/overlay@2.4.0-next.0

### Minor Changes

- feat: Allow to directly add sentry envelopes via trigger
([#498](#498))

## @spotlightjs/astro@2.1.7-next.0

### Patch Changes

-   Updated dependencies \[]:
    -   @spotlightjs/spotlight@2.3.2-next.0

## @spotlightjs/electron@1.1.7-next.0

### Patch Changes

-   Updated dependencies

\[[`87ba9ff9bd3e2634c599e123ee2fd11c26ba5ab6`](87ba9ff)]:
    -   @spotlightjs/overlay@2.4.0-next.0

## @spotlightjs/spotlight@2.3.2-next.0

### Patch Changes

-   Updated dependencies

\[[`87ba9ff9bd3e2634c599e123ee2fd11c26ba5ab6`](87ba9ff)]:
    -   @spotlightjs/overlay@2.4.0-next.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Okay, so for this to work properly, we need to do this at a higher level, not inside Sentry integration:

const listener = (event: MessageEvent): void => {

The name sidecar.ts is not great but essentially, that listener should be triggered which runs all integrations listening on the data and then updates their data with "setIntegrationData()`. This would allow bypassing sidecare much easier for other integrations too as it would provide a generic event ingestion system without the sidecar.

BYK added a commit that referenced this pull request Aug 26, 2024
This is the first step to send events into Spotlight UI through other means than the sidecar connection.

Related to #133 and #498.
@BYK
Copy link
Member

BYK commented Aug 26, 2024

Pushed out #506 which should make doing the right thing in this patch much easier. LMK if you want to do it yourself or okay me doing it :)

BYK added a commit that referenced this pull request Aug 27, 2024
This is the first step to send events into Spotlight UI through other
means than the sidecar connection.

Related to #133 and #498.
@mydea
Copy link
Member Author

mydea commented Aug 27, 2024

If you can do it, I'd greatly appreciate it - I am rather busy right now so not sure when I can get around to it 😅

BYK added a commit that referenced this pull request Aug 27, 2024
@BYK
Copy link
Member

BYK commented Aug 27, 2024

Should be done in #508. It still expects a full envelope in as a string or Buffer. Hoping that's a good-enough API? (directly analogous to the sidecar SSE format)

@mydea
Copy link
Member Author

mydea commented Aug 27, 2024

Should be done in #508. It still expects a full envelope in as a string or Buffer. Hoping that's a good-enough API? (directly analogous to the sidecar SSE format)

That should work just fine I believe, thank you! :)

@mydea mydea closed this Aug 27, 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