columnar: register metrics for hub#10941
Conversation
Signed-off-by: yongman <yming0221@gmail.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds engine-store stats access, one-time metrics startup, version-string prefix reporting, heartbeat disk-stat fallback, and build-time wiring for the columnar hub. ChangesStore Stats & Metrics Init
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
ti-chi-bot
Bot
added
the
size/S
Signed-off-by: yongman <yming0221@gmail.com>
ti-chi-bot
Bot
added
size/M
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contrib/tiflash-columnar-hub/.cargo/config.toml (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the rationale for allowing multiple symbol definitions.
The
--allow-multiple-definitionlinker flag weakens a standard safety check and can mask real symbol collisions. Given this is an FFI-heavy codebase interacting with TiKV/C++ components, the flag may be necessary due to duplicateextern "C"definitions across crate boundaries, but the reason is not self-evident from the config.Add a comment explaining:
- Why duplicates occur (e.g., metrics symbols, FFI helpers).
- That this is a temporary workaround or permanent architectural necessity.
- Any risks if definitions diverge.
As per coding guidelines, Rust projects should use
cargo clippyfor linting; while this is config, consider if the root cause can be fixed instead.🤖 Prompt for 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. In `@contrib/tiflash-columnar-hub/.cargo/config.toml` around lines 1 - 2, The Cargo config enables the linker’s allow-multiple-definition flag without explaining why, so add a clear comment near the rustflags setting in the tiFlash columnar hub config describing the specific duplicate symbol sources (for example shared FFI helpers or metrics symbols across crates), whether this is a temporary workaround or required by the current architecture, and the risk that divergent definitions could hide real collisions; also note in the surrounding context that if the duplicates are unintentional, the better fix is to resolve the root cause rather than relying on the linker workaround.Source: Coding guidelines
🤖 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 `@contrib/tiflash-columnar-hub/hub-runtime/src/run.rs`:
- Around line 940-945: The issue is in
collect_store_space_stats_from_engine_store: it currently returns Some even when
stats.fs_stats.capacity_size is zero, which prevents the fallback path in the
heartbeat logic from calling collect_store_space_stats(data_dir). Update this
helper to treat zero capacity as unavailable by returning None when fs_stats.ok
is zero or capacity_size is zero, so the existing fallback in the surrounding
store-space selection logic can use the local filesystem stats instead.
---
Nitpick comments:
In `@contrib/tiflash-columnar-hub/.cargo/config.toml`:
- Around line 1-2: The Cargo config enables the linker’s
allow-multiple-definition flag without explaining why, so add a clear comment
near the rustflags setting in the tiFlash columnar hub config describing the
specific duplicate symbol sources (for example shared FFI helpers or metrics
symbols across crates), whether this is a temporary workaround or required by
the current architecture, and the risk that divergent definitions could hide
real collisions; also note in the surrounding context that if the duplicates are
unintentional, the better fix is to resolve the root cause rather than relying
on the linker workaround.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9dc6fcf4-c0ac-4703-96f7-113cd1a55acc
📒 Files selected for processing (3)
contrib/tiflash-columnar-hub/.cargo/config.tomlcontrib/tiflash-columnar-hub/hub-runtime/src/engine_store_helper.rscontrib/tiflash-columnar-hub/hub-runtime/src/run.rs
Signed-off-by: yongman <yming0221@gmail.com>
There was a problem hiding this comment.
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 `@contrib/tiflash-proxy-cmake/CMakeLists.txt`:
- Around line 132-133: The ENABLE_NEXT_GEN_COLUMNAR branch in the CMakeLists
logic is hardcoding the Rust metric prefix, which breaks the existing build-time
override contract. Update the TIFLASH_RUST_ENV setup to read and pass through
the exported PROMETHEUS_METRIC_NAME_PREFIX value from the build
environment/Makefile instead of always injecting tiflash_proxy_, and keep the
change localized near the ENABLE_NEXT_GEN_COLUMNAR handling so the
Makefile/CMake integration remains consistent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fea451a0-2cc6-43c6-878f-534309e5631e
⛔ Files ignored due to path filters (1)
contrib/tiflash-columnar-hub/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
contrib/tiflash-columnar-hub/.cargo/config.tomlcontrib/tiflash-columnar-hub/Cargo.tomlcontrib/tiflash-columnar-hub/Makefilecontrib/tiflash-columnar-hub/hub-runtime/Cargo.tomlcontrib/tiflash-columnar-hub/hub-runtime/src/run.rscontrib/tiflash-proxy-cmake/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- contrib/tiflash-columnar-hub/.cargo/config.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/tiflash-columnar-hub/hub-runtime/src/run.rs
Signed-off-by: yongman <yming0221@gmail.com>
ti-chi-bot
Bot
added
size/L
ti-chi-bot
Bot
added
size/M
|
/test pull-unit-test |
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick release-nextgen-202603 |
|
@yongman: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
4 participants
What problem does this PR solve?
Issue Number: close #10943
Problem Summary:
Metrics are not registed to columnar hub runtime and the store stats not collected.
What is changed and how it works?
Register the metrics during startup and add store stats collect in store pd task.
Check List
Tests
Side effects
Documentation
Release note
Manual Test
Summary by CodeRabbit
Summary by CodeRabbit
Summary
New Features
PROMETHEUS_METRIC_NAME_PREFIX(default:tiflash_proxy_), and included it in the proxy’s version/debug output.Bug Fixes