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

Added lua-resty-http as sender #34

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

taythebot
Copy link

@taythebot taythebot commented Aug 5, 2020

I have added lua-resty-http as a sender. I was essentially having the same problem as issue #30.

The author of the previously mentioned issue did create his/her own pull request to add lua-resty-http as a sender, but the solution was overkill in my opinion. My module is modeled after the luasocket module and is only 2 functions.

I created this because of the following reasons:

One caveat of using lua-resty-http is that to use a custom SSL certificate, you will need do the following:

  1. Copy CA cert to a directory accessible by Nginx
  2. Put the following in your nginx.conf
lua_ssl_verify_depth 2;
lua_ssl_trusted_certificate <path_to_ca_cert>;

I would love to see this pull request merged!

Copy link
Contributor

@jdesgats jdesgats left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request! I'd by happy to merge it after the few comments I left are resolved :)

However, would it be possible to add at least a few basic tests to verify that the proposed implementation works?

@@ -15,6 +15,7 @@ to Sentry.]],
dependencies = {
"lua >= 5.1",
"lua-cjson",
"lua-resty-http"
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually not add senders dependencies on the rockspec because it doesn't make sense to every user of the module to install them. This is why luasocket and luasec are not listed here as they are specific to the luasocket backend.

})

if not res then
return nil, table_concat(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding request_uri returns a string error directly, not a table. Attempting to concatenate it will throw an error.

return nil, table_concat(err)
end

return true
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case the server responds a non-200 answer, the error would be silently ignored here. The raven library attempts to write the message in the error log as a last resort in case the sender cannot send it (https://github.com/cloudflare/raven-lua/blob/master/raven/init.lua#L323).


function mt:send(json_str)
local httpc = http.new()
local res, err = httpc:request_uri(self.server, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that sockets are not always available with lua_nginx_module: some phases such as init, set and log don't support async operations. The plain ngx module works around this limitation by spawning a timer that does the actual sending.

Perhaps there is a way to extract that logic so it doesn't need to be replicated here.

Defuse Venue added 2 commits August 8, 2020 16:12
- Removed lua-resty-htt pfrom rockspec
- Removed table_concat when returning error from request
- Added if statement to catch non 200 status code response
- Added ngx.timer support
@taythebot
Copy link
Author

taythebot commented Aug 8, 2020

Thank you for your pull request! I'd by happy to merge it after the few comments I left are resolved :)

However, would it be possible to add at least a few basic tests to verify that the proposed implementation works?

I've made the requested changes! I modeled the async send after ngx.lua but I had a few questions about how the library currently handles failed logs.

  1. In ngx.lua#L170, if the queue is full the new message is dropped and an error is returned. There is no function call to process the full queue. Shouldn't there be a call to process the full queue? Originally I was looking at the lua-resty-logger-socket library as an example and in socket.lua#L533 the library will process the full queue.

    My code currently does this, but I was wondering if there was a reason why this was not done in ngx.lua. Also would it useful to add the new message to a second queue to prevent it from being lost? Maybe it could be a user config to prevent any messages from being lost.

  2. In ngx.lua#L118, if the message fails to send an error is logged and the message is removed from the queue. Shouldn't it return on the log to prevent the message from being removed from the queue and lost?

I apologize in advance if these questions are annoying. This is my first open source contribution to the Lua community so I want to make sure it's proper!

Defuse Venue added 2 commits August 8, 2020 16:35
Changed ngx.log to util.errlog function
@jdesgats
Copy link
Contributor

In ngx.lua#L170, if the queue is full the new message is dropped and an error is returned. There is no function call to process the full queue. Shouldn't there be a call to process the full queue? Originally I was looking at the lua-resty-logger-socket library as an example and in socket.lua#L533 the library will process the full queue.

I haven't called anything to process the full queue because the background task is fired every time a message is pushed. When the queue fills up, the sending task is running, but the messages are pushed faster than they can be processed. Calling it can't hurt, but it is not going to change that fact either.

In ngx.lua#L118, if the message fails to send an error is logged and the message is removed from the queue. Shouldn't it return on the log to prevent the message from being removed from the queue and lost?

Well it depends of the kind of error: if the error is caused by a temporary external error (Sentry server down, network issue, ...) retrying later might work. However if the error is caused by the message itself (wrong syntax or schema, break size limit, ...), retrying will always fail, and will result in an error loop. Unfortunately, reliably differentiate these two cases is very hard.

From my point of view Sentry is a best-effort error reporting system, it is acceptable to loose a few messages from time to time. It also deals with the case where an error is triggered at a very high rate and the server cannot ingest them. In this case dropping the messages is the only reasonable thing to do.

Also note that the luacheck test is failing, you need to add the -- luacheck: globals ngx comment at the top of the file so it knows about the ngx global.

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.

2 participants