Container Parser Log Truncation at 1.6 MB when using multiline log recombination

[MoosaTae]

Component(s)

pkg/stanza

/pkg/stanza/operator/parser/container

What happened?

Description

Container logs processed by the OpenTelemetry Collector's container parser are being truncated at exactly 1,638,466 bytes (~1.56 MiB) when using multiline log recombination.

Steps to Reproduce

I use this config for log recombine logs multilines but I still found the split log

    receivers:
      filelog:
        include:
          - /var/log/pods/*/*/*.log
        operators:
          - id: container-parser
            type: container
          - id: recombine-multiline
            type: recombine
            combine_field: body
            combine_with: ""
            is_last_entry: body matches "\\n$"
            source_identifier: attributes["log.file.path"]
            max_batch_size: 10000
          - id: trim-trailing-newline
            type: regex_replace
            field: body
            regex: "[\n]+$"
            replace_with: ""
        start_at: end
        max_log_size: 10MiB
        storage: file_storage

Expected Result

It should recombine logs that larger than 1.56 MiB

Actual Result

it just split the logs because of hard limit on this defaultMaxBatchSize of container parser

Collector version

v0.138.0

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

receivers:
      filelog:
        include:
          - /var/log/pods/*/*/*.log
        operators:
          - id: container-parser
            type: container
          - id: recombine-multiline
            type: recombine
            combine_field: body
            combine_with: ""
            is_last_entry: body matches "\\n$"
            source_identifier: attributes["log.file.path"]
            max_batch_size: 10000
          - id: trim-trailing-newline
            type: regex_replace
            field: body
            regex: "[\n]+$"
            replace_with: ""
        start_at: end
        max_log_size: 10MiB
        storage: file_storage

Log output

Additional context

No response

Tip

_{React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[github-actions]

Pinging code owners:

• pkg/stanza: @andrzej-stencel

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[andrzej-stencel]

Thanks @MoosaTae for the report. cc: @ChrsMark as creator of the container operator

[ChrsMark]

Thank's!

We don't set the defaultMaxBatchSize at

opentelemetry-collector-contrib/pkg/stanza/operator/parser/container/config.go

Lines 121 to 126 in 02a0bf4

	recombineParserCfg := recombine.NewConfigWithID(recombineInternalID)
	recombineParserCfg.IsLastEntry = recombineIsLastEntry
	recombineParserCfg.CombineField = entry.NewBodyField()
	recombineParserCfg.CombineWith = ""
	recombineParserCfg.SourceIdentifier = entry.NewAttributeField(recombineSourceIdentifier)
	recombineParserCfg.MaxLogSize = c.MaxLogSize

indeed.

The parser initially followed what the Helm Chart's preset was using and this was not configured there: #31959

We can consider exposing this option specifically. However, I wonder if we should rather encapsulate+expose the whole recombine's config options instead of adding them one by one.

[MoosaTae]

Thank's!

We don't set the defaultMaxBatchSize at

opentelemetry-collector-contrib/pkg/stanza/operator/parser/container/config.go

Lines 121 to 126 in 02a0bf4
recombineParserCfg := recombine.NewConfigWithID(recombineInternalID)
recombineParserCfg.IsLastEntry = recombineIsLastEntry
recombineParserCfg.CombineField = entry.NewBodyField()
recombineParserCfg.CombineWith = ""
recombineParserCfg.SourceIdentifier = entry.NewAttributeField(recombineSourceIdentifier)
recombineParserCfg.MaxLogSize = c.MaxLogSize
indeed.

The parser initially followed what the Helm Chart's preset was using and this was not configured there: #31959

We can consider exposing this option specifically. However, I wonder if we should rather encapsulate+expose the whole recombine's config options instead of adding them one by one.

great Ideas, I will try add the entire recombine's config options

[andrzej-stencel]

I would rather not expose all recombine operator's configuration options in the container operator's config. Not all of them make sense, and we would couple the container operator's config to the internally used recombine operator config. In my view, the container operator using recombine operator internally is an implementation detail that should not be exposed to users 1) to prevent the cognitive load and the comprehensibility of the container operator and 2) to potentially allow the internal implementation to change without affecting the configuration surface.

We should only expose the options that make sense to the container operator. The operator already exposes the max_log_size option - shouldn't this be enough? Why would users also need to configure max_batch_size or max_unmatched_batch_size?

I propose to make the following changes:

1. Leave the existing configuration options on the container operator to users without changes - i.e. only make it possible to configure max_log_size. It is easy to understand and should be sufficient.
2. Remove the limit on the max batch size (max lines recombined) in the container operator's internal recombine operator instance. This should be possible by setting max_batch_size: 0, but this requires changes in the recombine operator, see below.
3. Set max_unmatched_batch_size to 0. I don't think there's any reason to not concatenate entries that were split by the container runtime.
4. Change the default on the max_log_size from 0 to an arbitrary value, to prevent logs from growing into infinity. Perhaps also make 0 an invalid value if we want to make sure logs do not grow into infinity.

For the above point 2. to work, the recombine operator needs to be modified to allow 0 value for the max_batch_size option, meaning there is no limit to the number of lines concatenated by the operator. Possibly a config validation rule should be added to prevent users from setting both max_batch_size and max_log_size to 0, to prevent infinite growing of log messages.

@ChrsMark and @MoosaTae please let me know your thoughts.

[MoosaTae]

I would rather not expose all recombine operator's configuration options in the container operator's config. Not all of them make sense, and we would couple the container operator's config to the internally used recombine operator config. In my view, the container operator using recombine operator internally is an implementation detail that should not be exposed to users 1) to prevent the cognitive load and the comprehensibility of the container operator and 2) to potentially allow the internal implementation to change without affecting the configuration surface.

We should only expose the options that make sense to the container operator. The operator already exposes the max_log_size option - shouldn't this be enough? Why would users also need to configure max_batch_size or max_unmatched_batch_size?

I propose to make the following changes:

1. Leave the existing configuration options on the `container` operator to users without changes - i.e. only make it possible to configure `max_log_size`. It is easy to understand and should be sufficient.

2. Remove the limit on the max batch size (max lines recombined) in the `container` operator's internal `recombine` operator instance. This should be possible by setting `max_batch_size: 0`, but this requires changes in the `recombine` operator, see below.

3. Set `max_unmatched_batch_size` to `0`. I don't think there's any reason to not concatenate entries that were split by the container runtime.

4. Change the default on the `max_log_size` from `0` to an arbitrary value, to prevent logs from growing into infinity. Perhaps also make `0` an invalid value if we want to make sure logs do not grow into infinity.

For the above point 2. to work, the recombine operator needs to be modified to allow 0 value for the max_batch_size option, meaning there is no limit to the number of lines concatenated by the operator. Possibly a config validation rule should be added to prevent users from setting both max_batch_size and max_log_size to 0, to prevent infinite growing of log messages.

@ChrsMark and @MoosaTae please let me know your thoughts.

I agree with you, for simplify common use cases just let recombine be unlimited seem make sense because of partial log if we apply container parser default use cases should recombine all of them, but for preventing unexpected behavior + edge cases like OOM or log recombine would go infinite, we should let the user possible to set that both max_batch_size and max_log_size

[andrzej-stencel]

@MoosaTae Looking at this condition https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.139.0/pkg/stanza/operator/transformer/recombine/transformer.go#L188-L190, isn't having non-zero max_log_size enough to prevent infinite growing of log messages?

[MoosaTae]

I think it's good to have for max_batch_size for protects against many tiny messages or missing combine_field cases (count-based)