feat: support not-equal matcher for PromQL metric names #5385
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces enhanced PromQL query capabilities for metric name matching in GreptimeDB. The changes add support for more complex metric name queries, including regex matching and not-equal comparisons. By introducing new dependencies, error handling mechanisms, and query processing logic, the implementation allows for more flexible metric name filtering across the database's Prometheus-compatible API. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PrometheusAPI
participant QueryHandler
participant CatalogManager
participant QueryEngine
Client->>PrometheusAPI: Send PromQL Query
PrometheusAPI->>QueryHandler: Process Query with Matchers
QueryHandler->>CatalogManager: Retrieve Table Information
CatalogManager-->>QueryHandler: Return Table Metadata
QueryHandler->>QueryEngine: Generate Logical Plan
QueryEngine->>QueryEngine: Execute Query
QueryEngine-->>QueryHandler: Return Metric Names
QueryHandler-->>PrometheusAPI: Return Filtered Metric Names
PrometheusAPI-->>Client: Respond with Results
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
docs-required
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/servers/src/prometheus.rs (1)
79-80: Handle potential empty conditions without using.unwrap()While the comment states that
conditionsmust not be empty, using.unwrap()can cause a panic if assumptions change. Consider handling the possibility of an emptyconditionsvector more safely.Apply this diff to replace
.unwrap()with proper error handling:// Safety: conditions MUST not be empty, reduce always return Some(expr). - let conditions = conditions.into_iter().reduce(Expr::and).unwrap(); + let conditions = conditions.into_iter().reduce(Expr::and).context(error::EmptyConditionsSnafu)?;src/servers/src/http/prometheus.rs (2)
143-145: Handle unsupportedPrometheusResponsevariants or provide appropriate feedbackIn the
appendmethod, non-matrix and non-vectorPrometheusResponsevariants are ignored in the_case. This might lead to silent failures or missed data when other variants are encountered. Consider handling these cases explicitly or logging a warning to aid in debugging.Apply this diff to log a warning for unsupported variants:
_ => { - // TODO(dennis): process other cases? + tracing::warn!("Unsupported PrometheusResponse variant encountered in append"); }
305-306: Add a comment to justify the use of.unwrap()in reduce operationThe call to
.unwrap()assumes thatresponsesis not empty. While previous checks ensure thatmetric_namesis not empty, makingresponsesnon-empty, adding a comment to explain this assumption improves code clarity.Consider adding the following comment:
// Safety: responses must contain at least one element as metric_names is not empty. responses .into_iter() .reduce(|mut acc, resp| { acc.data.append(resp.data); acc }) .unwrap()src/frontend/src/metrics.rs (1)
32-37: Add histogram buckets for better latency tracking.While the metric is well-defined, consider adding bucket definitions similar to
GRPC_HANDLE_QUERY_ELAPSEDfor more granular latency tracking.pub static ref PROMQL_QUERY_METRICS_ELAPSED: HistogramVec = register_histogram_vec!( "greptime_frontend_promql_query_metrics_elapsed", "frontend promql query metric names elapsed", - &["db"] + &["db"], + vec![0.005, 0.01, 0.05, 0.1, 0.5, 1.0, 5.0, 10.0, 60.0, 300.0] ) .unwrap();tests-integration/src/test_util.rs (1)
441-444: Consider adding error handling in the test utility function.While using
unwrap()is common in test code, consider adding basic error handling or at least error logging to help debug test failures.async fn run_sql(sql: &str, instance: &GreptimeDbStandalone) { - let result = instance.instance.do_query(sql, QueryContext::arc()).await; - let _ = result.first().unwrap().as_ref().unwrap(); + let result = instance.instance.do_query(sql, QueryContext::arc()).await; + if let Some(first) = result.first() { + if let Err(e) = first { + eprintln!("Failed to execute SQL: {}, error: {}", sql, e); + } + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
src/frontend/Cargo.toml(2 hunks)src/frontend/src/error.rs(3 hunks)src/frontend/src/instance.rs(3 hunks)src/frontend/src/instance/promql.rs(1 hunks)src/frontend/src/metrics.rs(1 hunks)src/servers/Cargo.toml(1 hunks)src/servers/src/http/prometheus.rs(9 hunks)src/servers/src/lib.rs(1 hunks)src/servers/src/prometheus.rs(1 hunks)src/servers/src/prometheus_handler.rs(2 hunks)tests-integration/src/test_util.rs(2 hunks)tests-integration/tests/http.rs(2 hunks)
🔇 Additional comments (12)
src/servers/src/prometheus_handler.rs (1)
36-41: LGTM! Well-documented trait method addition.The new async method signature is well-designed for querying metric names with matchers, aligning perfectly with the PR objectives.
src/servers/src/lib.rs (1)
43-43: LGTM! Clean module addition.The new prometheus module is properly declared and follows the file's alphabetical ordering convention.
src/frontend/src/error.rs (3)
172-177: LGTM! Well-structured error variant.The
CollectRecordbatcherror variant follows the established pattern and includes proper error source tracking.
234-239: LGTM! Appropriate error handling for metric names query.The
PrometheusMetricNamesQueryPlanerror variant properly handles errors from the prometheus metric names query plan creation.
366-370: LGTM! Proper status code mapping.The status code mappings for both new error variants are appropriate and consistent with the existing error handling patterns.
tests-integration/src/test_util.rs (1)
454-467: LGTM! Well-structured test data setup.The SQL commands are well organized:
- Creates a physical table first
- Creates metric tables referencing the physical table
- Inserts test data with appropriate timestamps
src/frontend/src/instance.rs (2)
23-23: LGTM! Clean module addition.The new
promqlmodule is properly declared.
455-464: LGTM! Well-implemented error handling.The implementation properly maps errors using BoxedError and follows the codebase's error handling patterns.
tests-integration/tests/http.rs (2)
701-724: LGTM! Comprehensive test coverage for the new matcher functionality.The test cases effectively cover:
- Positive case with regex matching multiple metrics
- Specific case matching single metric
- Response content validation
616-616: LGTM! Field name validation added.Simple but effective validation of field name values.
src/frontend/Cargo.toml (1)
40-40: LGTM! Dependencies properly added.The new dependencies:
- Use workspace versioning for consistency
- Are alphabetically ordered
- Support the new PromQL functionality
Also applies to: 51-51
src/servers/Cargo.toml (1)
105-105: LGTM! Verify dependency usage.The addition of
store-apias a workspace dependency follows the project's dependency management pattern.Let's verify that this dependency is actually used in the codebase:
✅ Verification successful
Dependency verified and properly used ✅
The
store-apidependency is used to access schema-related constants necessary for implementing the metric name matcher and table limit features.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for store-api usage in the servers crate rg "use store_api::" src/servers/Length of output: 109
Script:
#!/bin/bash # Check the specific imports and their usage rg "use store_api::metric_engine_consts::" -A 3 src/servers/src/http/prometheus.rsLength of output: 195
There was a problem hiding this comment.
Copilot reviewed 6 out of 13 changed files in this pull request and generated no comments.
Files not reviewed (7)
- src/servers/Cargo.toml: Evaluated as low risk
- src/frontend/src/metrics.rs: Evaluated as low risk
- src/servers/src/lib.rs: Evaluated as low risk
- src/servers/src/prometheus_handler.rs: Evaluated as low risk
- src/frontend/src/error.rs: Evaluated as low risk
- src/frontend/Cargo.toml: Evaluated as low risk
- src/frontend/src/instance.rs: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/servers/src/http/prometheus.rs:864
- The function
update_metric_name_matchershould handle nested matchers to ensure all matchers are correctly updated.
/// Update the `__name__` matchers in expression into special value
Co-authored-by: Ruihang Xia <[email protected]>
killme2008
force-pushed
the
feature/promql-query-name
branch
from
e396177 to
06d782a
Compare
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
Close #5375
What's changed and what's your intention?
Supports regex matches (or not) for
__name__in Prometheus APIs, currently limited to/api/v1/query_rangeand/api/v1/query.Caps the query metric tables at a maximum of 1024.
PR Checklist
Please convert it to a draft if some of the following conditions are not met.
Summary by CodeRabbit
Release Notes
New Features
Performance
Dependencies
Error Handling
These changes expand the Prometheus query capabilities and improve overall system robustness.