Skip to content

ref(relay): Limit maximum number of logs produced by expansion - #6263

Open
klochek wants to merge 1 commit into
masterfrom
christopherklochek/ingest-1079-security-report-relay-otlp-logs-resourcescope-attributes
Open

ref(relay): Limit maximum number of logs produced by expansion#6263
klochek wants to merge 1 commit into
masterfrom
christopherklochek/ingest-1079-security-report-relay-otlp-logs-resourcescope-attributes

Conversation

@klochek

@klochek klochek commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@klochek
klochek requested a review from a team as a code owner July 27, 2026 13:09
@linear-code

linear-code Bot commented Jul 27, 2026

Copy link
Copy Markdown

INGEST-1079

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 409e55f. Configure here.

return Err(TooManyExpandedLogs);
}

Ok(())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Debug quantity mismatch on limit

Medium Severity

When expansion hits TooManyExpandedLogs, produce has already called records.modify_by for each log emitted so far, but expand returns None and process::expand still finishes try_map with empty logs via unwrap_or_default. Debug RecordKeeper::success then fails quantity balancing and panics.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 409e55f. Configure here.

Comment on lines 641 to +642
pub max_log_size: ByteSize,
/// The maximum number of logs that can result from a log expansion.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

max_expanded_log_count is only enforced for integrations, not log containers

The new max_expanded_log_count limit only applies to log integrations (OTel, Vercel, NEL) and not to LogItems::Container. A container with many small logs bypasses the count cap entirely, causing unbounded per-request CPU and memory usage during normalization, filtering, and scrubbing.

Evidence
  • relay-server/src/processing/logs/process.rs:expand() receives max_expanded_log_count but only passes it to integrations::expand() for LogItems::Integration.
  • For LogItems::Container, it calls expand_log_container() with no count parameter, so the cap is never checked.
  • A container is bounded by max_container_size (default 12 MB), but with small logs that still allows tens or hundreds of thousands of items—far above the max_expanded_log_count default of 1000.
  • Every log from the container then goes through validate::size, normalize, filter, scrub, and normalize_derived with no early count-based rejection, unlike the integration path where produce() returns Err(TooManyExpandedLogs) once the cap is exceeded.

Identified by Warden · wrdn-dos-review · MG4-3M7

Comment on lines +46 to +48
if logs.len() > max_expanded_log_count {
return Err(TooManyExpandedLogs);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this check isn't enough, as we're already deserializing (either via serde_json or prost) the entire array.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So, it's the entire array, but we've only expanded (with accompanying attributes) logs.len() full log entries. It's the amplification by the attributes that can make this size a lot larger than we'd like.

@Dav1dde Dav1dde Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Parsing is already too much. The amplification of attributes is just a bigger multiplier. The OurLog struct is 208 bytes, the minimum size for a valid log item is 2 bytes in JSON (3 if we count the ,, 2 in protobuf). That is a 100x memory multiplier without considering the attribute amplification. With a size limit of 12 MiB per container, that means in a parsed state this is ~1 GiB. Then we have additional overheads like WithHeader<OurLog> and over-allocation of the Vec (capacity). To fully address the issue we already need to limit the count during parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants