feat(summary view): added list metric details apis | 7084 #7086
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 0ca886e in 2 minutes and 32 seconds
More details
- Looked at
304lines of code in7files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. pkg/query-service/utils/filter_conditions.go:1
- Draft comment:
Overall, the filter conditions code is clear and concise. As a minor suggestion, consider declaring and reusing constants for the ClickHouse functions used (e.g.like,notLike,match) to ensure consistency and easier maintenance. Also, ensure there are thorough unit tests covering all operator cases. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The suggestion about constants is specific and actionable - it would make the code more maintainable by centralizing ClickHouse function names. However, the unit test suggestion is vague and falls under the "verify/ensure" category we should avoid. The code is new, so these changes are relevant to the diff.
The comment combines a good suggestion with a problematic one. Also, is introducing constants really worth the extra abstraction for these simple string literals?
The constants suggestion is still valuable because it would prevent typos and make updates easier if ClickHouse function names change. We can ignore the testing part.
Keep a modified version of the comment that focuses only on the constants suggestion, removing the testing part.
2. pkg/query-service/utils/filter_conditions.go:25
- Draft comment:
Casting item.Operator to string here: ensure that item.Operator is reliably convertible to a string and that no unexpected values occur. Consider validating its type before conversion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code already has error handling for invalid operators through the switch statement's default case. The type conversion appears safe since Go's type system would catch any incompatible types at compile time. The comment is asking for validation that's already effectively present in the code.
I might be overlooking potential runtime panics if item.Operator is of an unexpected type that can't be safely converted to string.
In Go, type mismatches would be caught at compile time, and the switch statement already handles invalid values. The code is already defensive enough.
The comment should be deleted as it suggests adding validation that's already effectively present through compile-time checks and the error handling in the switch statement.
3. pkg/query-service/utils/filter_conditions.go:27
- Draft comment:
When wrapping the value in '%' for contains operator, ensure that ClickHouseFormattedValue properly escapes special characters to prevent potential SQL injection issues. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the author to ensure that special characters are properly escaped to prevent SQL injection issues. It is not a direct suggestion or a specific request for a change, but rather a general caution. This violates the rule against asking the author to ensure behavior is intended or tested.
4. pkg/query-service/utils/filter_conditions.go:31
- Draft comment:
The determination of isJSONKey based on item.FilterTypeKey defaults to false for unhandled cases. Verify that this fallback is intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment asks the PR author to verify if the fallback behavior is intended, which violates the rule against asking for confirmation of intention. It does not provide a specific suggestion or improvement.
5. pkg/query-service/utils/filter_conditions.go:85
- Draft comment:
The cases for FilterOperatorContains and FilterOperatorLike both result in a 'like(...)' expression. Consider consolidating these cases if they are intended to behave identically. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment appears to miss the important distinction between Like and Contains. Like allows custom patterns while Contains wraps the value with wildcards for substring matching. These are distinct use cases that shouldn't be consolidated. The comment could lead to incorrect refactoring that would break functionality.
Maybe there's still value in noting the code duplication, even if the operators behave differently? The SQL generation could potentially be refactored to share code.
While there is some code duplication, it's minimal (single line) and consolidating would make the code less clear. The current separation makes the distinct behaviors more obvious.
The comment should be deleted because it incorrectly suggests consolidating two operators that have intentionally different behaviors.
6. pkg/query-service/utils/filter_conditions.go:89
- Draft comment:
For the EXISTS / NOT EXISTS operators, the condition always uses JSONExtractKeys(labels). Ensure that this mechanism is correct when the field isn’t a JSON key. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that a mechanism is correct, which violates the rule against asking the author to ensure behavior is intended. It doesn't provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
7. pkg/query-service/utils/filter_conditions.go:60
- Draft comment:
Consider adding unit tests covering all switch branches of buildSingleFilterCondition to verify that generated SQL snippets are syntactically correct. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. pkg/query-service/utils/filter_conditions.go:60
- Draft comment:
Using fmt.Sprintf to build SQL conditions can be brittle. Consider adopting a query builder or parameterized queries to increase maintainability and safety. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While using string formatting for SQL can be dangerous due to SQL injection, this code is specifically for ClickHouse and has safeguards. The values are pre-formatted using ClickHouseFormattedValue(). The operators are strictly typed enums. The keys are either direct column names or JSON paths. Without seeing ClickHouseFormattedValue implementation, we can't be fully certain about safety.
I might be overlooking potential SQL injection vectors. The implementation of ClickHouseFormattedValue is crucial but not visible.
While SQL injection is a valid concern, the code shows evidence of proper value escaping and type safety through the use of enums and a dedicated formatting function.
The comment should be deleted as the code already shows evidence of proper safety measures, and without more context about ClickHouseFormattedValue, we can't definitively say there's an issue.
Workflow ID: wflow_VyckSjzAM5xmYm2r
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
reviewed
Feb 12, 2025
ee/query-service/app/api/api.go
Outdated
| @@ -217,4 +217,7 @@ func (ah *APIHandler) MetricExplorerRoutes(router *mux.Router, am *baseapp.AuthM | |||
| router.HandleFunc("/api/v1/metrics/{metric_name}", | |||
| am.ViewAccess(ah.GetMetricsDetails)). | |||
| Methods(http.MethodGet) | |||
| router.HandleFunc("/api/v1/metrics", | |||
| metric_name, | ||
| COUNT(*) AS data_points, | ||
| MAX(unix_milli) AS last_received_time | ||
| FROM %s.%s |
There was a problem hiding this comment.
We need to use the time series and samples table based on the time range
| WHERE unix_milli BETWEEN ? AND ? | ||
| GROUP BY metric_name | ||
| ) AS s USING (metric_name) | ||
| WHERE t.unix_milli BETWEEN ? AND ? AND |
There was a problem hiding this comment.
We should filter first and then join
| SELECT | ||
| t.metric_name AS metric_name, | ||
| argMax(t.description, t.fingerprint) AS description, | ||
| argMax(t.type, t.fingerprint) AS type, |
There was a problem hiding this comment.
What is argMax doing? There is any https://clickhouse.com/docs/en/sql-reference/aggregate-functions/reference/any aggregate function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added list metrics api.
Related Issues / PR's
#7084
Important
Add new API endpoint to list metric details with supporting database queries and utilities.
ListMetricsendpoint inapi.gofor listing metric details.ListMetricsfunction insummary.goto handle requests.ListSummaryMetricsmethod inreader.goto query ClickHouse for metric summaries.BuildFilterConditionsinfilter_conditions.goto construct query conditions.ListMetricsWithSummaryinsummary.goto callListSummaryMetrics.ParseSummaryListMetricsParamsinparser.goto parse API request parameters.Readerinterface ininterface.goto includeListSummaryMetrics.This description was created by
for 0ca886e. It will automatically update as commits are pushed.