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

short_message not appearing #5

Open
ErisDS opened this issue Jul 3, 2019 · 6 comments
Open

short_message not appearing #5

ErisDS opened this issue Jul 3, 2019 · 6 comments

Comments

@ErisDS
Copy link

ErisDS commented Jul 3, 2019

I'm in the process of testing out log4js-node as a replacement for bunyan, and checking that the gelf transport works as expected.

I'm using gelf to send data to logstash. It's working for the most part, except that short_message doesn't appear in kibana. If I change the code to use _short_message instead, it works as expected. I've tried several different layouts - currently testing with dummy, but I've tested basic and the default.

My apender looks like this:

gelf: {type: '@log4js-node/gelf', host: 'xxx', port: 12201, facility: 'log-test', layout: {type: 'dummy'}, customFields: {_foo: 'bar'}}

This is really weird. The facility property gets sent correctly, timestamp, host, level etc all work.

According to the logstash docs short_message is expected, and it works with the existing bunyan setup.

I can't see any important difference between the code in this repo vs the gelf-stream package used for bunyan.

I also checked gelf-stream's dep gelfling and it's not adding the _.

I am still looking for configuration issues that could be causing this, but I'm pretty confused because the messages are being sent to a different index but the same server, with all the same config as the working bunyan version.

Any ideas?

@nomiddlename
Copy link
Contributor

From the appender code, I can't see why short_message would not be included. Could you post a full code snippet that reproduces the problem, please?

@ErisDS
Copy link
Author

ErisDS commented Jul 4, 2019

I've gotten a little further with debugging this. The problem is layered, with small issues with my config, the logstash gelf input plugin, and this appender all conflating into a tricky-to-unpick situation. This extremely long comment walks through the whole thing and ends with a proposed small change to the appender.

First here's my minimal node snippet:

const log4js = require('log4js');

log4js.configure(
    {
        appenders: {
            gelf: {type: '@log4js-node/gelf', host: 'xxx', port: 12201, facility: 'log-test', layout: {type: 'dummy'}, customFields: {_foo: 'bar', _full_message: null}}
        },
        categories: {default: {appenders: ['gelf'], level: 'debug'}}
    });

const logger = log4js.getLogger('default');

logger.info('hello world');

My logstash config did have a bunch of filters, including a mutation to remove message which was causing problems. For testing I've now changed the config to be just:

input {
    gelf {
        add_field => {
            gelf_collector => "xxx"
        }
    }
}

output {
    stdout {}
}

Given this config, I'm expecting messages to come through with a short_message of "hello world", but instead they're coming through with a message of "hello world".

This explains why they weren't appearing before, the mutation to remove message was unexpectedly removing them. So why are they appearing as message instead of short_message?

This is due to the code in the logstash gelf input plugin:

https://github.com/logstash-plugins/logstash-input-gelf/blob/master/lib/logstash/inputs/gelf.rb#L264

if event.get("full_message") && !event.get("full_message").empty?
      event.set("message", event.get("full_message").dup)
      event.remove("full_message")
      if event.get("short_message") == event.get("message")
        event.remove("short_message")
      end
    elsif event.get("short_message") && !event.get("short_message").empty?
      event.set("message", event.get("short_message").dup)
      event.remove("short_message")
    end

If full_message is included in the log, it is copied to message and full_message is removed - if short_message matches, it's also removed. Else if there's no full_message but there is a short_message, short_message is copied to message and removed.

@log4js-node/gelf doesn't have support for full_message, so short_message ends up being copied and removed.


In order to get to the behaviour I'm looking for, I need to be able to send full_message. Unfortunately, given the way @log4js-node/gelf is written, this is currently not possible - even if I set full_message it won't get passed through, I have to use _full_message, which then doesn't get treated the same.

I believe the easiest fix would be to change the customField code slightly, to have a list of allowed gelf fields that can be passed through without an underscore. This is the same approach as is used in gelfling:

list of fields: https://github.com/mhart/gelfling/blob/master/gelfling.js#L8
pass through: https://github.com/mhart/gelfling/blob/master/gelfling.js#L103

@nomiddlename would you be up for pushing out this change if I PR it?

@nomiddlename
Copy link
Contributor

I don't think the GELF appender is doing anything wrong here, according to the GELF payload spec. full_message is an optional field (although I'm not sure what you would put in there that would be different to short_message most of the time). short_message is mandatory. Looks like the logstash-input-gelf plugin is doing the right thing, converting the full_message field to message (the logstash equivalent) if available, otherwise falling back to short_message which should always be available in a GELF payload. What was the purpose of the filter that was removing the message field? Sounds like that is the problem.

I'm not trying to avoid making changes in this library, just trying to make sure we make the right ones - at the moment this sounds like a very specific problem that's related to the way your logstash is configured and I don't know how widely this applies.

@nomiddlename
Copy link
Contributor

Oh, there's also the logstashUDP appender for log4js which might help you. Or at least give you a whole different set of problems :)

@ErisDS
Copy link
Author

ErisDS commented Jul 4, 2019

full_message is an optional field

What I’m proposing is a 2 line change adding support for this part of the spec in a backwards compatible way, that will save me an inordinate about of work.

To provide context, the point of my work is trying to swap the now unmaintained Bunyan for a new library in our incumbent infrastructure, which features many micro services and large numbers of instances. I’m trying to reach feature parity so that I don’t also have to handle incompatibilities in our existing elk stack whilst we roll this update across our full infra - and to reduce data inconsistencies.

This bug and log4js-node/log4js-node#911 are currently blockers to us making the switch in Ghost.

Happy to put in the leg work to PR the fix, but I’d really appreciate your support before sinking any more time into it.

@nomiddlename
Copy link
Contributor

Ok, so a user would be able to do something like this?

logger.info({ GELF: true, full_message: 'something really long' }, 'this is the short message');

Is that how you're going to use it?

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

No branches or pull requests

2 participants