All newlines in short messages are replaced with whitespaces about logback-gelf HOT 10 CLOSED

The current implementation does indeed seem to be doing too much. Would you like to send a pull request that changes that without breaking the class’s interface?

from logback-gelf.

Yes, I would send a pull request, but I need additional information.

What is the expected behavior of normalizeShortMessage()?

• It should replace empty strings with a something like "Empty message replaced by logback-gelf".
• It should replace blank strings alike.
  See GelfCodec in graylog2-server

if (StringUtils.isBlank(shortMessageNode.asText()) && StringUtils.isBlank(messageNode.asText())) {
    throw new IllegalArgumentException(prefix + "has empty mandatory \"short_message\" field.");
}

This could simply be implemented like this:

@Override
public String normalizeShortMessage(String shortMessage) {
    if (shortMessage.isBlank()) {
        addWarn("Log message was blank - replaced to prevent Graylog error");
        return "Blank message replaced by logback-gelf";
    }
    return shortMessage;
}

• Should it still trim leading and trailing whitespaces like the current implementation does? (Simply return shortMessage.trim();)
  This can be helpful in some situations, but I don't think this is enforced by the GELF specification. Therefore we should not always enforce this. It could be an additional configurable feature (later).

• It should probably not replace any whitespace char with ' ' like the current implementation does? This is the nature of this issue. But I don't know the initial motivation for implementing it this way.

from logback-gelf.

• It should shorten the string to maxShortMessageLength.

from logback-gelf.

I'd say, it should:

• Strip leading whitespaces (using String#stripLeading)
• Shorten the string to maxShortMessageLength, if longer than that
• Strip trailing whitespaces (using String#stripTrailing)
• Replace blank strings with something like "Empty message replaced by logback-gelf"

Stripping whitespaces could be optional (configurable).

from logback-gelf.

I just created the PR #101, which does not include the option to strip inner whitespaces.

But I wonder, if we should rethink the design:

• The PatternLayout formats the ILoggingEvent to a string.
  • This could include stripping or squashing of whitespaces.
  • This could include trimming to a max length.
  • Some of this is already possible with PatternLayout and an appropriate pattern (trimming like %.-250(%m) ).
  • Allowing custom implementations of Layout provides further flexibility.
• The GelfEncoder encodes that string and additional information to a json message, conforming to the GELF spec.
  • The already formatted string remains unchanged.
  • Except the case of blank strings, that are replaced with "Empty message replaced by logback-gelf".

from logback-gelf.

PR #101 is now mergeable.

What do you think of my previous comment?

from logback-gelf.

What do you think of my previous comment?

I think, you're absolutely right! I built the whole sanitising logic based on the misunderstanding of Graylog2/graylog2-server#4842

Formatting the message itself (including shortening, trimming, etc.) should be the sole responsibility of the pattern layout. Only the blank string handling has to be addresses – as you pointed out.

Could you create another PR for that?

from logback-gelf.

PR #102 is mergeable.

Will there be a patch or minor release soon?

from logback-gelf.

Will there be a patch or minor release soon?

A major release (due to breaking changes) is planned within the next two weeks.

from logback-gelf.

Just released 6.0.0 - thanks for your contribution!

from logback-gelf.