Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Async submission by default #4

Closed
taylortrimble opened this issue Dec 9, 2013 · 8 comments
Closed

Async submission by default #4

taylortrimble opened this issue Dec 9, 2013 · 8 comments

Comments

@taylortrimble
Copy link
Contributor

Currently, the Sentry docs encourage you to do async submission whenever possible:

Additionally, the following features are highly encouraged:

  • Automated error handling (e.g. default error handlers)
  • Logging integration (to whatever standard solution is available)
  • Non-blocking event submission
  • Basic data sanitization (e.g. filtering out values that look like passwords)

This is the desirable configuration in 99% of production cases, but it means that simple "hey look I can send" demos don't work if you don't make your goroutine sleep.

The raven-python client only has a send method, and automatically decides whether or not to send asynchronously based on the transport. I think we should do this as well. We could supply an option to disable it, but that would still trip up newbies just trying to send and oh-my-it-works.

I recommend making the change as a highly desirable default, and updating the docs and example. We could also consider making that example show up as a step-by-step demo in the README. I could do that.

Whatchya think?

@titanous
Copy link
Contributor

titanous commented Dec 9, 2013

I'm quite happy with the explicitness of Send vs Report. I think the docs need to be more clear, but I don't think combining them makes much sense. In Go all of your underlying transports are going to be async to some extent, and 99% of users will use the HTTP transport, which is async.

@taylortrimble
Copy link
Contributor Author

I can get behind that. Any purpose in keeping Report around then? If it's conventional for golang transports to be asynchronous by default we could probably get rid of it.

@titanous
Copy link
Contributor

Do you mean Send? Report is the async one. I'm okay with dropping Send if we provide a channel that returns an error value so that Report can optionally be used synchronously. The ID from the response should also be made accessible.

@taylortrimble
Copy link
Contributor Author

I just figured out where some confusion is!

It's based the truthiness of these statements: "A synchronous send to Sentry waits for the HTTP response and parses the returned "id". An asynchronous send does not. Therefore, when you want the "id" you do a sync send, and when you don't you do an async send."

However, I'm not convinced that's the way to think about it, and I'm feeling thorough: I've got 2️⃣ reasons Sentry isn't built this way, 3️⃣ examples of it not being built that way, and 1️⃣ demonstrated behavior of why the 3 examples work. Buckle your seat belts. 😉

Sentry isn't built this way

  1. The docs don't actually encourage Reading the Response of anything but the status code
  2. UDP. UDP is a valid transport, and you don't get a response from sentry's UDP server

Examples of how the eventID is actually returned

  1. raven-python
  2. raven-php
  3. raven-csharp

The first two have a form of return $data['event_id']; where $data is what's passed to it, and the last one explicitly documents that the expected behavior is returning the request event_id, not the response's. The commit message reads: "Made the documentation in RavenClient.Capture methods explicit about using JsonPacket.EventID as its return value", though it admittedly doesn't do this: interestingly enough, the conflict was introduced by the same person on the same day. 😝 But it seems to be the consensus that you should return what you send, not what you receive.

Why this works

Sentry doesn't prevent duplicate IDs. No really! I made this change, and every response I got back from the Sentry server was the ID I sent it, even if it was a duplicate. In other words, like the docs say, you only need to read the response code from the response: the ID it sends back will always be the one you sent it:

tylrtrmbl:example % go run example.go http://<pubkey>:<pvtkey>@sentry.thenewtricks.com/4
{"id": "0123456789abcdef0123456789abcdef"}2013/12/27 14:39:54 sent packet successfully
tylrtrmbl:example % go run example.go http://<pubkey>:<pvtkey>@sentry.thenewtricks.com/4
{"id": "0123456789abcdef0123456789abcdef"}2013/12/27 14:40:02 sent packet successfully
tylrtrmbl:example % 

To conclude my novel

I think we should mimic raven-python's architecture, since it's basically the golden master template for a Sentry client and it works:

  1. Async is introduced by the transports, not the "send" methods
  2. You prepare for any kind of async transport by assuming you don't get a response - you parse your ID return value from the packet you send

This is exactly the way #2 works now, since our HTTP transport is async by default as given to us by the go std library. Whaddaya think, heavy reader? :bowtie:

Fin

@titanous
Copy link
Contributor

Okay, so clients generate the ID if they want it? We should probably add that.

I miscommunicated the asyncness of our current HTTP transport. The Send method on the transport is actually asynchronous, it is made async by the worker goroutine and the channel used in Report.

My inclination is to keep it close to the existing model of having the Send method on transports be blocking until a final error can be returned. This allows the synchronicity to be managed by the Client rather than creating two options (sync/async) for each of two protocols (HTTP/UDP).

@titanous
Copy link
Contributor

Hmm, we could do a wrapper transport that's async.

@titanous
Copy link
Contributor

Also solved by #2?

@taylortrimble
Copy link
Contributor Author

Yeppers!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants