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(http-log) add buffering support using a generic facility #3604

Merged
merged 3 commits into from Jul 18, 2018

Conversation

Projects
None yet
2 participants
@hishamhm
Copy link
Member

hishamhm commented Jul 10, 2018

This PR introduces a generic logging buffer module, refactored out of the Galileo-specific logging buffer and uses it in the http-log plugin.

This adds three new optional properties to the configuration of http-log:

  • retry_count (default value: 10)
  • queue_size (default value: 1)
  • flush_timeout (default value: 2)

Their behaviors are the same as the identically-named options in the galileo plugin.

Unlike in galileo, the default of queue_size is set to 1 so that the default behavior of http-log remains backwards-compatible.

The output format of http-log is dependent on the queue_size parameter:

  • When queue_size is 1, it makes one HTTP request per logged entry, containing a single JSON object in each. This is compatible with the previous releases of the plugin.
  • When queue_size is greater than 1, the HTTP connection will send a JSON array containing one or more JSON objects in it, up to the configured queue size.

The new fields are not required, and when absent their defaults mean that they behave the same as the http-log plugin always did, meaning this requires no migrations.

This PR also fixes the http-log test suite, which was entirely disabled due to reliance on an unavailable feature of mockbin.com. It adds a mock log server to our custom Nginx configuration of the test suite.

Includes a test case for http-log buffering using the mock log server.

@hishamhm hishamhm requested review from bungle and kikito Jul 11, 2018

-- Sender object for using the Generic Logging Buffer.


local http = require "resty.http"

This comment has been minimized.

Copy link
@kikito

kikito Jul 16, 2018

Member

Spacing: this file needs blank lines before all the elses and elseifs.

function serializer.new(conf)
if type(conf) ~= "table" then
return nil, "arg #1 (conf) must be a table"
elseif type(conf.service_token) ~= "string" then

This comment has been minimized.

Copy link
@kikito

kikito Jul 16, 2018

Member

Spacing: add blank lines before elseifs.

local buffer_serializer
if conf.queue_size > 1 then
-- Serialize messages into a JSON array
buffer_serializer = JSONSerializer.new(true)

This comment has been minimized.

Copy link
@kikito

kikito Jul 16, 2018

Member

Blank line needed

-- @return `parsed_url` a table with host details like domain name, port, path etc
local function parse_url(host_url)
local parsed_url = url.parse(host_url)
if not parsed_url.port then

This comment has been minimized.

Copy link
@kikito

kikito Jul 16, 2018

Member

The body of this if is not correctly indented, and the internal elseif needs a blank line before it

This comment has been minimized.

Copy link
@hishamhm

hishamhm Jul 16, 2018

Author Member

Most of this is code (especially in buffer.lua above) pulled straight from the Galileo plugin buffer. The git blame of the file was unfortunately lost when the file move and the code changes I made were squashed in a single commit (Git assumes the file was "rewritten").

I will fixup the commit with some cleanups.


if type(conf) ~= "table" then
return nil, "arg #2 (conf) must be a table"
elseif conf.retry_count ~= nil and type(conf.retry_count) ~= "number" then

This comment has been minimized.

Copy link
@kikito

kikito Jul 16, 2018

Member

Blank lines before elseif

@@ -195,6 +195,10 @@ build = {
["kong.plugins.oauth2.daos"] = "kong/plugins/oauth2/daos.lua",
["kong.plugins.oauth2.api"] = "kong/plugins/oauth2/api.lua",

["kong.plugins.log-buffering.json_serializer"] = "kong/plugins/log-buffering/json_serializer.lua",

This comment has been minimized.

Copy link
@kikito

kikito Jul 16, 2018

Member

The naming is at odds with other previous pieces, and is somewhat confusing.

Since the json_serializer and lua_serializer both are called "serializer", that makes me think that they have something to do with kong.plugins.log-serializers.basic. (But then, why the differences in location and names)

Upon looking at their source code, I see that they are completely different. I would recommend finding a different word than "serializer" for them (maybe "producers"?)

This comment has been minimized.

Copy link
@hishamhm

hishamhm Jul 16, 2018

Author Member

I reused the naming of the Galileo log buffering module, which used serializers. I agree it is confusing with log-serializers. Thanks for the suggestion, will switch to "producers".

end

local self = {
buffer = {},

This comment has been minimized.

Copy link
@kikito

kikito Jul 16, 2018

Member

I find troublesome the fact that the serializers have an attribute called buffer but at the same time they are required by a Buffer. I think the Buffer could be renamed to Queue to make it a bit less confusing.

This comment has been minimized.

Copy link
@hishamhm

hishamhm Jul 16, 2018

Author Member

the Buffer objects also have a sending queue, so that's not much better. :) Renaming those to self.output :)

@hishamhm hishamhm force-pushed the feat/generic-log-buffer branch from 4886fcb to d1b3a46 Jul 16, 2018

@hishamhm

This comment has been minimized.

Copy link
Member Author

hishamhm commented Jul 16, 2018

@kikito addressed all concerns, please re-review

@kikito

kikito approved these changes Jul 17, 2018

@hishamhm hishamhm changed the base branch from master to next Jul 17, 2018

hishamhm added some commits Jul 10, 2018

tests(http-log) re-enable http-log tests using new mock log server
The testsuite for http-log was marked as disabled due to dependency
on `mockbin.com`.

This commit reenables it, by implementing a mock log server in
our `mock_upstream` server block at `spec/fixtures`.
refactor(plugins) add a generic logging buffer
This commit creates a generic logging buffer module, refactored out
of the Galileo-specific logging buffer.
feat(http-log) adds log buffering to http-log plugin
Adds three new properties to the configuration of http-log:

* `retry_count` - default 10
* `queue_size` - default 1
* `flush_timeout` - default 2

Their behaviors are the same as the identically-named options
in the `galileo` plugin.

The default of `queue_size` is set to 1 so that the default behavior
of the plugin remains backwards-compatible.

When `queue_size` is 1, it makes one HTTP request per logged
entry, containing a single JSON object in each.

When `queue_size` is greater than 1, the HTTP connection will send
a JSON array containing one or more JSON objects in it, up to
the configured queue size.

The new fields are not required, and when absent their defaults
mean that they behave the same as the http-log plugin always did,
meaning this requires no migrations.

Includes a test case using the mock log server.

@hishamhm hishamhm force-pushed the feat/generic-log-buffer branch from d1b3a46 to 41a5a65 Jul 17, 2018

@hishamhm hishamhm merged commit de4a002 into next Jul 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hishamhm hishamhm deleted the feat/generic-log-buffer branch Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.