Skip to content

feat(loggly): support HTTP response code to SYSLOG severity mapping#6228

Merged
spacewander merged 4 commits into
apache:masterfrom
bisakhmondal:feat/loggly/resp-severity
Feb 15, 2022
Merged

feat(loggly): support HTTP response code to SYSLOG severity mapping#6228
spacewander merged 4 commits into
apache:masterfrom
bisakhmondal:feat/loggly/resp-severity

Conversation

@bisakhmondal

Copy link
Copy Markdown
Member

What this PR does / why we need it:

closed #6112 (phase 2 step 2)

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

tzssangglass
tzssangglass previously approved these changes Jan 28, 2022
Comment thread apisix/plugins/loggly.lua Outdated

if conf.severity_map then
for k, v in pairs(conf.severity_map) do
local rcode = tonumber(k)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can do part of the validation in jsonschema? Like

patternProperties = {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Comment thread apisix/plugins/loggly.lua
end
end

local message_severity = severity[conf.severity:upper()]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do the conversion in check_schema so we don't need to do it per request.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Comment thread apisix/plugins/loggly.lua
if not rcode or rcode < 100 or rcode >= 600 then
return nil, "expecting severity_map with http response code([100,599]) as keys"
end
cache[k] = severity[v:upper()]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can convert k to a number?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think I avoid that due to the default behaviour of cjson encoding Cannot serialise table: excessively sparse array.
we can override the behaviour by cjson.encode_sparse_array(true). In this case, I thought I don't need it that's why I am sticking with strings. WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK

Comment thread apisix/plugins/loggly.lua Outdated
type = "object",
description = "upstream response code vs syslog severity mapping",
patternProperties = {
[".*"] = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can use regex to verify the range of status code?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yea, I had tested. It doesn't work that way.
say for eg,
["^[1-5][0-9]{2}"] = {...} the check schema match the enums for only the set of keys where the key is a status code (regex). For other scenarios (key = "ABC", "500a") there is no matching regex, so it simply doesn't match and validate with the pattern and let it pass as valid inputs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does ["^[1-5][0-9]{2}$"] = {...} work for it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@spacewander It works as well as doesn't work.
Example 1: Want ✔️ Got: ✔️

...
           "severity_map": {
                "200": "INFO"
            }
...

Example 2: Want ❌ Got: ❌

...
           "severity_map": {
                "200": "abc"
            }
...

Example 3: Want ❌ Got: ✔️ :

...
           "severity_map": {
                "abc": "def"
            }
...

The problem is, if the key fails to match regex, the pattern properties don't get applied. Also, the program doesn't throw an error for the key mismatch.

@spacewander spacewander Feb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it. What about adding additionalProperties = false,? So the mismatch key will cause an error.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It worked! Awesome : )

// inp "severity_map": {
//               "abc": "def"
//            }
{
    "error_msg": "failed to check the configuration of plugin loggly err: property \"severity_map\" validation failed: additional properties forbidden, found abc"
}

@spacewander
spacewander merged commit 0b9a5fb into apache:master Feb 15, 2022
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.

Proposal: Develop Loggly logging plugin

3 participants