nklapste commented 5 years ago

Resolves the issues present with the GELFUDPHandler not properly dealing with the case of a chunked GELF message overflowing.

Adds three GELF chunker classes each having different behavior on a GELF chunk overflow on a message:

BaseGELFChunker silently drops GELF messages that chunk overflow
GELFWarningChunker notes a python Warning and drops GELF messages that chunk overflow
GELFTruncatingChunker notes a python Warning and simplifies and truncates a chunk overflowing GELF message in a attempt sending to at least send some content to Graylog

Additionally, users can subclass BaseGELFChunker to implement custom handling of chunk overflowing GELF messages.

Other features:

Misc refactoring
Improvement on instrumentation testing stability
Documentation improvements

resolves #116 resolves #112

nklapste commented 5 years ago

@gabrieljardim and @higherorderfunctor could you please review if these changes meet your needs?

nklapste commented 5 years ago

Whats your opinion on refactoring the chunker classes and GELFUDPHandler into each other? These classes seem closely coupled (at least in the GELFTruncatingChunker's case).

higherorderfunctor commented 5 years ago

I can test out the GELFTruncatingChunker tomorrow as that appears to meet my use case.

higherorderfunctor commented 5 years ago

I haven't been able to get this to work with the GELFRabbitHandler. Small messages work fine, but large messages appear to silently drop. I am not very knowledgeable of the GELF or AMQP protocols, so I am not even sure if chunking like this works in this case? I have tried with compression on and off.

from graypy import GELFRabbitHandler as BaseGELFRabbitHandler
from graypy.handler import GELFTruncatingChunker, BaseGELFChunker

class CustomGELFRabbitHandler(BaseGELFRabbitHandler):
    chunker: BaseGELFChunker

    def __init__(
            self, *args: Any,
            chunker: BaseGELFChunker = GELFTruncatingChunker(),
            **kwargs: Any
    ) -> None:
        super().__init__(*args, **kwargs)
        self.chunker = chunker

    def makePickle(self, record: LogRecord) -> bytes:
        message_dict = self._make_gelf_dict(record)
        data = json.dumps(message_dict, cls=AnyTypeJsonEncoder)
        return str.encode(data)

    def send(self, s: bytes) -> None:
        if len(s) < self.chunker.chunk_size:
            super(CustomGELFRabbitHandler, self).send(s)
        else:
            for chunk in self.chunker.chunk_message(s):
                super(CustomGELFRabbitHandler, self).send(chunk)

nklapste commented 5 years ago

@higherorderfunctor

Correct me if I am wrong. I'm not very well versed in RabbitMQ or AMQP.

I don't think using GELF chunking is applicable for a GELFRabbitHandler. RabbitMQ's transport is based off of TCP. And GELF chunking is designed only for UDP Graylog inputs (thus, it is only supported for the GELFUDPHandler).

GELF chunking is only noted within the Graylog documentation under the "GELF via UDP" section (see: https://docs.graylog.org/en/3.1/pages/gelf.html#gelf-via-udp).

Also, Graylog's documentation under the "GELF via TCP" section (see: https://docs.graylog.org/en/3.1/pages/gelf.html#gelf-via-tcp) states that TCP Graylog inputs only support uncompressed and non-chunked payloads.

I would assume sending chunked GELF messages via means other than a UDP Graylog input would yield unexpected results.

On a side note: RabbitMQ and AMQP is designed to send high-reliability messaging (with guaranteed delivery no matter what). Is the native GELFRabbitHandler failing to send large GELF messages in your use case? If so something more sinister may be wrong.

higherorderfunctor commented 5 years ago

I was hitting the 32KiB field limit on the message which was getting rejected by graylog. I was under the assumption the entire message had to be 32KiB, but now I'm seeing it's per indexed field. Capping field lengths is probably not within the scope of this PR and is essentially what I'm doing in the issue I posted. Unless you wanted to report/handle oversized fields to the user, I'm fine with my issue being closed.

codecov-io commented 5 years ago

Codecov Report

Merging #118 into master will increase coverage by 1.28%. The diff coverage is 98.46%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   94.69%   95.97%   +1.28%     
==========================================
  Files           3        3              
  Lines         226      273      +47     
==========================================
+ Hits          214      262      +48     
+ Misses         12       11       -1

Impacted Files	Coverage Δ
graypy/rabbitmq.py	`86.2% <100%> (ø)`	:arrow_up:
graypy/handler.py	`99.51% <98.43%> (+0.76%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e2782c7...5bbea4d. Read the comment docs.

nklapste commented 4 years ago

@severb care to re-review?

Afterwards I'm going to merge this PR and likely release a new build of graypy under version 2.0.0 (this PR and a previous PR have some awkward parameter changes that could potentially be backwards breaking).

severb commented 4 years ago

@severb care to re-review?

Afterwards I'm going to merge this PR and likely release a new build of graypy under version 2.0.0 (this PR and a previous PR have some awkward parameter changes that could potentially be backwards breaking).

LGTM 👍

gabrieljardim commented 4 years ago

@gabrieljardim and @higherorderfunctor could you please review if these changes meet your needs?

Done!

severb / graypy

Feature/handle chunk overflow #118

Codecov Report