Skip to content

opentelemetry: fix JSON histogram bounds handling - #11989

Merged
edsiper merged 2 commits into
masterfrom
otlp-histogram-bounds-ghsa-5cg6-8p7m-5rgq
Jun 24, 2026
Merged

opentelemetry: fix JSON histogram bounds handling#11989
edsiper merged 2 commits into
masterfrom
otlp-histogram-bounds-ghsa-5cg6-8p7m-5rgq

Conversation

@edsiper

@edsiper edsiper commented Jun 24, 2026

Copy link
Copy Markdown
Member

Problem

OTLP/HTTP JSON metrics can carry histogram explicitBounds that are not monotonically non-decreasing. The JSON decoder passed those bounds into cmetrics before rejecting them, and then freed the bucket object again when cmt_histogram_create() returned NULL.

Changes

  • Validate JSON histogram explicit bounds in the OpenTelemetry metrics decoder before creating cmetrics buckets.
  • Treat cmt_histogram_create() as consuming the buckets object once called, including internal failure paths.
  • Add an in-opentelemetry integration regression test that rejects descending bounds and verifies the listener remains healthy by accepting a subsequent valid histogram.

Validation

  • cmake --build build --target fluent-bit-bin -j 8
  • tests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_in_opentelemetry_rejects_json_histogram_with_descending_bounds -q
  • VALGRIND=1 VALGRIND_STRICT=1 tests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_in_opentelemetry_rejects_json_histogram_with_descending_bounds -q
  • ctest --test-dir build -R flb-it-opentelemetry --output-on-failure
  • GITHUB_EVENT_NAME=pull_request GITHUB_BASE_REF=master tests/integration/.venv/bin/python .github/scripts/commit_prefix_check.py

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved OTLP JSON histogram validation to reject explicitBounds that are not in non-decreasing order, including degenerate or invalid bound sequences.
    • Refined histogram construction and error handling so failures during bucket setup are handled safely and consistently.
  • Tests

    • Added an integration test covering rejection of descending/invalid histogram explicitBounds and acceptance of valid bounds, verifying stored datapoint values (including normalized explicitBounds).

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 16a0def1-b46c-44d9-a73c-95127eb3d61c

📥 Commits

Reviewing files that changed from the base of the PR and between 4ec5ede and c7a1438.

📒 Files selected for processing (2)
  • src/opentelemetry/flb_opentelemetry_metrics.c
  • tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/opentelemetry/flb_opentelemetry_metrics.c

📝 Walkthrough

Walkthrough

The OTLP JSON histogram decoder gains a non-decreasing order check for explicit histogram bounds. A new C helper rejects null or out-of-order bounds arrays, an early-return error path frees temporary allocations on failure, and the cmt_histogram_create failure cleanup is refactored to allocate buckets separately. An integration test covers both the rejection and acceptance paths.

Changes

Histogram bounds ordering validation

Layer / File(s) Summary
Bounds-order helper, validation call, and error-path cleanup
src/opentelemetry/flb_opentelemetry_metrics.c
Adds check_histogram_bounds_order to detect null or non-monotone bounds, calls it after parsing explicit bounds with cleanup on failure, and refactors cmt_histogram_create to allocate buckets separately with corrected failure paths that destroy the histogram instance on bucket allocation failure.
Integration test: reject descending, accept ascending bounds
tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py
Adds build_histogram_json_payload for parameterized OTLP JSON histogram payloads and test_in_opentelemetry_rejects_json_histogram_with_descending_bounds, which asserts HTTP 400 and no stored metrics for descending bounds, then verifies correct histogram datapoint fields for ascending bounds.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • cosmo0920

Poem

🐇 Hoppy bounds, in order laid,
From low to high, the histogram's made.
Descending bounds? A 400 you get!
Ascending bounds? The bits are set.
The rabbit checks each pair in line—
No out-of-order mess, all fine! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing JSON histogram bounds handling in the OpenTelemetry metrics decoder by validating explicit bounds order before creating cmetrics buckets.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch otlp-histogram-bounds-ghsa-5cg6-8p7m-5rgq

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@edsiper
edsiper marked this pull request as ready for review June 24, 2026 01:42
@edsiper
edsiper requested a review from cosmo0920 as a code owner June 24, 2026 01:42
@edsiper edsiper added this to the Fluent Bit v5.0.8 milestone Jun 24, 2026

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a1156599c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

buckets,
metric_label_count,
(char **) metric_label_keys);
buckets = NULL;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep freeing buckets when create rejects arguments

When the JSON histogram has a valid bucket array but an empty metric name, cmt_histogram_create() returns NULL during its argument checks before it attaches or destroys the passed buckets; setting buckets = NULL unconditionally here skips the existing cleanup in the histogram == NULL path and leaks one bucket object per rejected request. Only transfer ownership after the call is known to have consumed the object, or free buckets on the pre-consumption failure path.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/opentelemetry/flb_opentelemetry_metrics.c`:
- Around line 1819-1822: The bounds validation in `check_histogram_bounds_order`
currently uses a greater-than comparison operator which allows equal adjacent
bounds to pass, but the OTLP specification requires bounds to be strictly
increasing. Modify the comparison operator in the bounds order validation
condition from greater-than to greater-than-or-equal to enforce strictly
increasing bounds and align with the OTLP proto contract specification, similar
to how `cmt_histogram_buckets_validate` implements this validation.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 173654ec-ed91-4aa2-b010-f39bf72f4ae5

📥 Commits

Reviewing files that changed from the base of the PR and between a115659 and 4ec5ede.

📒 Files selected for processing (2)
  • src/opentelemetry/flb_opentelemetry_metrics.c
  • tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py

Comment thread src/opentelemetry/flb_opentelemetry_metrics.c
edsiper added 2 commits June 23, 2026 20:03
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
@edsiper
edsiper force-pushed the otlp-histogram-bounds-ghsa-5cg6-8p7m-5rgq branch from 4ec5ede to c7a1438 Compare June 24, 2026 02:03
@edsiper
edsiper merged commit 7806758 into master Jun 24, 2026
59 of 61 checks passed
@edsiper
edsiper deleted the otlp-histogram-bounds-ghsa-5cg6-8p7m-5rgq branch June 24, 2026 02:50
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.

1 participant