Skip to content

Fix max_message_length truncation for multibyte messages - #285

Open
JSap0914 wants to merge 1 commit into
fluent-plugins-nursery:masterfrom
JSap0914:fix-260-byte-safe-truncation
Open

Fix max_message_length truncation for multibyte messages#285
JSap0914 wants to merge 1 commit into
fluent-plugins-nursery:masterfrom
JSap0914:fix-260-byte-safe-truncation

Conversation

@JSap0914

Copy link
Copy Markdown

What

  • truncate formatted messages by UTF-8 byte length instead of character count
  • discard an incomplete multibyte character at the truncation boundary
  • add regression coverage for exact and split multibyte boundaries

Why

CloudWatch validates event size in bytes. Character-based truncation could therefore leave multibyte messages larger than max_message_length, while a raw byte slice could produce invalid UTF-8.

Fixes #260

Tests

  • bundle exec ruby -Itest test/plugin/test_out_cloudwatch_logs.rb --name test_truncate_message_uses_byte_limit_and_preserves_utf8 (1 test, 5 assertions)
  • CI=true bundle exec ruby -Itest -e 'Dir["test/plugin/*.rb"].sort.each { |file| require File.expand_path(file) }' (58 tests, 57 assertions, 46 expected omissions)
  • gem build fluent-plugin-cloudwatch-logs.gemspec --output /tmp/fluent-plugin-cloudwatch-logs-worker5.gem
  • git diff --check

Comment on lines +342 to +344
return message unless @max_message_length

message.byteslice(0, @max_message_length).scrub("")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add an early return when no truncation is needed.

Suggested change
return message unless @max_message_length
message.byteslice(0, @max_message_length).scrub("")
return message unless @max_message_length
return message if message.bytesize <= @max_message_length
message.byteslice(0, @max_message_length).scrub("")

As written, every event pays for a full string copy plus a full scrub scan even when the message already fits. max_message_length is typically configured with a fairly large value, so the common case is that nothing needs to be truncated.

Besides the performance win, this keeps scrub("") scoped to the messages we actually cut. scrub("") removes invalid byte sequences anywhere in the string rather than only at the boundary, so without the early return, setting max_message_length would silently alter messages that need no truncation at all. In practice scrub_record! already replaces invalid bytes in record values with U+FFFD before formatting, so this only matters for a custom <format> section that emits invalid UTF-8, but there is no reason to touch those messages when we are not truncating them.

@Watson1978 Watson1978 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please update README.

This is a behavior change for users with multibyte messages, since their messages will now be truncated earlier than before. The current wording is ambiguous about the unit:

* `max_message_length`: maximum length of the message

Could you update it to state that the limit is measured in UTF-8 bytes, and mention that a character split across the boundary is dropped rather than corrupted? Something like maximum length of the message in UTF-8 bytes (a multibyte character split at the boundary is dropped) would work.

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.

Different sizing methods can cause

2 participants