Skip to content

Comments

Make http request async#13

Open
yjh0502 wants to merge 1 commit intoasabil:masterfrom
yjh0502:http_async
Open

Make http request async#13
yjh0502 wants to merge 1 commit intoasabil:masterfrom
yjh0502:http_async

Conversation

@yjh0502
Copy link
Contributor

@yjh0502 yjh0502 commented Aug 19, 2014

raven:capture/2 blocks until http request finishes. It can slow
down error_logger when raven is used with {error_logger, true}
because error_logger waits until raven_capture/2 finishes.

Currently raven:capture/2 does not check return value of http
request so it's safe to make http request async.

@yjh0502
Copy link
Contributor Author

yjh0502 commented Feb 22, 2015

Any comment?

@dcramer
Copy link

dcramer commented Feb 22, 2015

@yjh0502 raven-erlang isnt a super widely used thing unfortunately. That said, and knowing nothing about erlang, async defaults are +1 from me

@asabil
Copy link
Owner

asabil commented Feb 22, 2015

I am not quite sure to be honest, the changes you suggest will lead to errors being silently dropped and your logs being lost, also it you are removing back pressure which means that you might end up with memory usage ballooning if you are producing more logs than what sentry can ingest.

@yjh0502
Copy link
Contributor Author

yjh0502 commented Feb 22, 2015

@asabil Errors are already silently dropped because there is no return value check on httpc:request. If we need to check errors, we can handle error in both sync/async cases.
With current implementation memory usage is ballooning when reporting errors to sentry via error_logger, because error_logger is buffering errors for raven-erlang. httpc:request can takes hundreds of milliseconds only with network latency when using hosted sentry service at other continents, in this case throughput of whole error logging system is limited to <10 tps.

How about adding raven:capture/3 which takes option argument? It can take [async] or [{timeout, 1000}] as options. For back-pressure, we can add timeout to httpc:request which effectively limits memory usage. I'll prepare a patch for this.

raven:capture/2 blocks until http request finishes. It can slow
down error_logger when raven is used with {error_logger, true}
because error_logger waits until raven_capture/2 finishes.

To prevent blocking, add raven:capture/3 that takes third option
argument. If users give [async] to option, error is sent asynchronously
without blocking. [async] could be set with config, like {[async, true]}
if users want to send all logs in async.

With async logging, memory usage with httpc:request can increase
indefinitely. To prevent memory balloning, users can set timeout value
of httpc:request. Default timeout if 5000ms, which is same with default
timeout of gen_server. Users can change timeout with either
configuration parameter like [{timeout, 1000}] or raven:capture/3 with
[{timeout, 1000}] as options.

Note that default behavior is not changed. All logs are sent
synchronously if there is no configurations/options are given.
@asabil
Copy link
Owner

asabil commented Feb 22, 2015

Sounds good, however I would suggest adding the options to the DSN, in a similar manner to other Sentry clients (for example: https://github.com/getsentry/raven-java)

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