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

Is it okay than sentry/electron transport drops all info from response and sentry/core base-transport tries to check it? #942

Closed
3 tasks done
origin-yaropolk opened this issue Jul 8, 2024 · 7 comments · Fixed by #943

Comments

@origin-yaropolk
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Electron SDK Version

5.1.0

Electron Version

30.1.2

What platform are you using?

Windows

Link to Sentry event

No response

Steps to Reproduce

Expected Result

I want full info in response.

Actual Result

I see some dissonance in the fact that, on the one hand, the sentry removes information from the response, and on the other hand, it tries to immediately check it.

Also, in this case, events like 'afterSendEvent' doesn't make sense with empty response.

image

@lforst
Copy link
Member

lforst commented Jul 9, 2024

From what I can tell the electron SDK will return the headers which is everything the SDK needs. The fact that the status code isn't returned doesn't matter except for logging. I'll let @timfish confirm though!

@timfish
Copy link
Collaborator

timfish commented Jul 9, 2024

I've tried withstatusCode included and the tests all pass so maybe the question is why not include this if it impacts afterSendEvent?

@lforst
Copy link
Member

lforst commented Jul 9, 2024

Just to be conscious around API surface I think. The more we expose the more can break. And this doesn't seem necessary 🤔 @origin-yaropolk what is your use case?

@timfish
Copy link
Collaborator

timfish commented Jul 9, 2024

Oh ok, if we don't return the statusCode, the only thing this appears to impact outside of afterSendEvent is outputting this warning:

          // We don't want to throw on NOK responses, but we want to at least log them
          if (response.statusCode !== undefined && (response.statusCode < 200 || response.statusCode >= 300)) {
            DEBUG_BUILD && logger.warn(`Sentry responded with status code ${response.statusCode} to sent event.`);
          }

@timfish
Copy link
Collaborator

timfish commented Jul 9, 2024

I've opened a PR to pass through the status code because it's low risk and it seems like this was expected at some point.

It's worth noting that the Electron SDK's default transport has an offline cache. This means that if the event cannot be sent, it is saved to retry later and a default status code of 200 is returned. For this reason, the status code returned by the transport is not a clear indicator of the sending status!

@origin-yaropolk
Copy link
Author

And this doesn't seem necessary 🤔 @origin-yaropolk what is your use case?

I don’t have a specific use case.
I came across this inconsistency when I was looking for a way to get rid of event spam when the quota is over.
image

@origin-yaropolk
Copy link
Author

I’am sure that old sdk version below 3.0.0 had another behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants