Skip to content

Conversation

@nikhilsinhaparseable
Copy link
Contributor

@nikhilsinhaparseable nikhilsinhaparseable commented Jan 14, 2026

remove metrics_ and exemplar_ prefixes in otel-metrics

When key collision happens i.e. attributes at two different hierarchies in the incoming event have same attribute, the later overrides the previous attribute entry in the ingested event.

We will add this behaviour to the documentation.

Summary by CodeRabbit

  • Refactor
    • Streamlined internal attribute handling logic to improve code maintainability and consistency.

Note: This release contains no user-visible changes. Updates are internal to system architecture only.

✏️ Tip: You can customize this high-level summary in your review settings.

remove `metrics_` and `exemplar_` prefixes in otel-metrics

When key collision happens i.e. attributes at two different hierarchies in the incoming event have same attribute,
the later overrides the previous attribute entry in the ingested event.

We will add this behaviour to the documentation.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 14, 2026

Walkthrough

The change consolidates attribute insertion logic in OpenTelemetry metrics handling by replacing two specific helper functions (insert_metric_attributes and insert_exemplar_attributes) with a single generic insert_attributes function, removing automatic key prefixing and reducing code duplication.

Changes

Cohort / File(s) Summary
OpenTelemetry Metrics Refactoring
src/otel/metrics.rs
Removes insert_metric_attributes and insert_exemplar_attributes helper functions; replaces all call sites with generic insert_attributes function; removes KeyValue import; JSON attribute keys no longer auto-prefixed with metric/exemplar identifiers

Possibly related PRs

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Hops through metrics with delight,
One helper function shines so bright,
Prefixes gone, the code runs lean,
Cleanest attributes ever seen!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description addresses the core change but lacks required sections from the template: missing detailed goal statement, solution rationale, key changes explanation, and the testing/documentation checklist. Add the required template sections including explicit description of the goal, rationale for the chosen approach, comprehensive list of key changes, and complete the testing and documentation checklist items.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'consolidate attributes insertion in otel-metrics' directly aligns with the main change of replacing specific attribute insertion functions with a generic insert_attributes helper.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac1e6e0 and 8d19593.

📒 Files selected for processing (1)
  • src/otel/metrics.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T01:31:41.786Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1415
File: src/metadata.rs:63-68
Timestamp: 2025-08-25T01:31:41.786Z
Learning: The TOTAL_EVENTS_INGESTED_DATE, TOTAL_EVENTS_INGESTED_SIZE_DATE, and TOTAL_EVENTS_STORAGE_SIZE_DATE metrics in src/metadata.rs and src/storage/object_storage.rs are designed to track total events across all streams, not per-stream. They use labels [origin, parsed_date] to aggregate by format and date, while per-stream metrics use [stream_name, origin, parsed_date] labels.

Applied to files:

  • src/otel/metrics.rs
🧬 Code graph analysis (1)
src/otel/metrics.rs (1)
src/otel/otel_utils.rs (1)
  • insert_attributes (185-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
  • GitHub Check: coverage
🔇 Additional comments (3)
src/otel/metrics.rs (3)

73-114: Good: exemplar attributes are inserted before exemplar system fields (system fields win on collisions).
This ordering makes collisions like an attribute named exemplar_trace_id deterministic (the computed trace id wins).


120-170: Collision semantics changed: datapoint attrs can now be overwritten by exemplar attrs/fields (and vice-versa) depending on insertion order.
Given the prefix removal, please ensure the “last write wins” behavior here is the intended documented precedence (especially if multiple exemplars exist, since later exemplars also overwrite earlier ones during the flatten+merge loop).


448-587: The proposed reordering reverses a recently-committed intentional design decision. Commit 8d19593 explicitly establishes that attributes should override system fields at the same hierarchy level ("later overrides previous"). The current code order—system fields first, then insert_attributes()—correctly implements this intent and was scheduled for documentation.

If you intend to change this precedence strategy, that's a separate architectural decision beyond this PR. The current implementation is correct relative to the documented intent.

Regarding cross-level precedence (resource > scope > metric > datapoint): This remains an open design question. The code currently implements "resource wins" (later merge overrides). Please clarify the intended semantics and ensure test coverage reflects it.

Likely an incorrect or invalid review comment.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

@nitisht nitisht merged commit b25cdfa into parseablehq:main Jan 14, 2026
12 checks passed
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.

2 participants