Skip to content

fix: correct competitive bench line counting for OTLP and NDJSON#613

Merged
strawgate merged 2 commits into
masterfrom
copilot/worktree-2026-04-01T16-00-26
Apr 1, 2026
Merged

fix: correct competitive bench line counting for OTLP and NDJSON#613
strawgate merged 2 commits into
masterfrom
copilot/worktree-2026-04-01T16-00-26

Conversation

@strawgate
Copy link
Copy Markdown
Owner

@strawgate strawgate commented Apr 1, 2026

Summary

  • fix blackhole line counting used by competitive benchmarks
  • count OTLP /v1/logs records by parsing resourceLogs[].scopeLogs[].logRecords[]
  • keep fallback to "body": scan when OTLP JSON parsing is unavailable
  • fix NDJSON counting for payloads without trailing newline
  • add regression tests for NDJSON, ES bulk, and OTLP log record counting

Why

Benchmark throughput can be under-reported when payloads do not end with \n (notably affecting jsonline senders like vlagent), and OTLP payload counting should use log-record semantics rather than newline heuristics.

Validation

  • cargo test -p logfwd-competitive-bench
  • cargo check -p logfwd-competitive-bench
  • just ci in this environment is blocked by pre-existing cargo deny duplicate warnings unrelated to this patch

Notes

I attempted focused docker benchmark runs for otelcol and vlagent, but benchmark execution is blocked/hangs in this environment. The code path and regression tests are in place for deterministic counting behavior.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 1, 2026

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

The blackhole benchmark's request counting for /v1/logs now tries OTLP JSON structural counting by parsing the body and summing resourceLogs[].scopeLogs[].logRecords[]. If parsing yields no count it falls back to the prior substring "body:" occurrence scan, and if that still yields zero it uses an NDJSON-style estimator (ndjson_line_count) which returns 0 for empty bodies and adds 1 when the body lacks a trailing newline. NDJSON/newline handling and OTLP counting unit tests were added.

Possibly related PRs


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/logfwd-competitive-bench/src/blackhole.rs`:
- Around line 109-116: The OTLP counting currently calls count_otlp_log_records
and count_otlp_body_keys unconditionally; change to lazy-evaluate the fallback
so you only run the heavier substring/scan when the JSON parse returned no
records: first call count_otlp_log_records(body) and if it returns a positive
count return it immediately; only if it returns 0 then call
count_otlp_body_keys(body) and return that if >0; otherwise fall back to
ndjson_line_count(body, newlines). Update the logic around the otlp_count/return
path (the block guarded by url.contains("/v1/logs")) to reflect this
short-circuit behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f33bedf7-c108-4817-84e2-5c993dbb012a

📥 Commits

Reviewing files that changed from the base of the PR and between 6380b91 and 3c17fe5.

📒 Files selected for processing (1)
  • crates/logfwd-competitive-bench/src/blackhole.rs

Comment thread crates/logfwd-competitive-bench/src/blackhole.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@strawgate strawgate merged commit 7229bdc into master Apr 1, 2026
6 of 8 checks passed
@strawgate strawgate deleted the copilot/worktree-2026-04-01T16-00-26 branch April 1, 2026 18:04
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.

1 participant