-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/7077 | Related metrics api #7149
base: feat/7080
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 843d9e1 in 3 minutes and 1 seconds
More details
- Looked at
615
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. pkg/query-service/app/metricsexplorer/summary.go:150
- Draft comment:
Dashboard info is obtained by marshalling then unmarshalling JSON; consider a direct transform to improve performance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/app/summary.go:107
- Draft comment:
Check error when reading request body before reassigning r.Body. Re-reading may mask underlying errors. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/utils/format.go:340
- Draft comment:
Using math.Pow with float conversion in GetEpochNanoSecs can be brittle; consider an integer-based calculation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/query-service/utils/format.go:156
- Draft comment:
Clean up the inline comment formatting in QuoteEscapedStringForContains; remove extra spaces in the URL reference. - Reason this comment was not posted:
Confidence changes required:40%
<= threshold50%
None
5. pkg/query-service/interfaces/interface.go:135
- Draft comment:
Ensure downstream callers adhere to the DAO pattern for any non-ClickHouse functions; the GetRelatedMetrics method addition appears appropriate. - Reason this comment was not posted:
Comment was on unchanged code.
6. pkg/query-service/utils/format.go:162
- Draft comment:
Remove extra space in the comment URL in QuoteEscapedStringForContains (e.g. "https: //clickhouse...") to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/utils/format.go:306
- Draft comment:
Document the rationale for truncating typeName by one character in GetClickhouseColumnName; the slicing (typeName[:len(typeName)-1]) is non-obvious. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pkg/query-service/utils/format.go:262
- Draft comment:
Consider simplifying pointer dereferencing in getPointerValue using reflect.Indirect to cover all pointer cases in a generic way. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/utils/format.go:209
- Draft comment:
Instead of using fmt.Sprint and strings.Fields for formatting numeric slices in ClickHouseFormattedValue, consider iterating and formatting each element to preserve formatting control. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/utils/format.go:339
- Draft comment:
Clarify expected input units and scale in GetEpochNanoSecs; adding documentation about whether it expects seconds, milliseconds, etc. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/utils/format.go:187
- Draft comment:
Consider controlling the precision when formatting float values in ClickHouseFormattedValue to avoid overly long decimals. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_O7TFifSVr06gMUPl
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -621,3 +621,97 @@ func GetDashboardsWithMetricName(ctx context.Context, metricName string) ([]map[ | |||
|
|||
return result, nil | |||
} | |||
|
|||
func GetDashboardsWithMetricNames(ctx context.Context, metricNames []string) (map[string][]map[string]string, *model.ApiError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider refactoring the existing GetDashboardsWithMetricName
function to use this more general version.
- function
GetDashboardsWithMetricName
(model.go)
// Here we're using 0.5 for each, but adjust as needed. | ||
finalScores := make(map[string]float64) | ||
for metric, scores := range relatedMetricsMap { | ||
finalScores[metric] = scores.NameSimilarity*0.5 + scores.AttributeSimilarity*0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weights (0.5 for name and attribute similarity) are hardcoded. Consider declaring them as constants for clarity and easier future adjustments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on ead25be in 2 minutes and 21 seconds
More details
- Looked at
32
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
15
drafted comments based on config settings.
1. pkg/query-service/app/clickhouseReader/reader.go:6148
- Draft comment:
Avoid building SQL queries by concatenating strings (e.g. via fmt.Sprintf) with user-controlled values (e.g. ruleID and metric names). Use parameterized queries wherever possible to prevent SQL injection risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/app/clickhouseReader/reader.go:6170
- Draft comment:
This file has grown too large and covers multiple responsibilities (metrics, logs, traces, rule state history, live tail, etc.). Consider refactoring by splitting the file into smaller modules to improve maintainability and readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/app/clickhouseReader/reader.go:6240
- Draft comment:
Multiple functions (e.g. LiveTailLogsV3/V4) share almost identical logic. Consider consolidating duplicate logic into common helper functions to reduce code duplication. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/query-service/app/clickhouseReader/reader.go:4244
- Draft comment:
The use of reflect to scan SQL rows in readRow/readRowsForTimeSeriesResult increases complexity and may impact performance. Consider using strongly typed structs or helper libraries to handle row scanning safely. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/query-service/app/clickhouseReader/reader.go:5614
- Draft comment:
When concatenating lists of metric names (or other identifiers) into queries, make sure to properly sanitize these values. String-joining values directly can lead to injection if not carefully controlled. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/query-service/app/clickhouseReader/reader.go:5564
- Draft comment:
Avoid constructing IN clauses by concatenating unsanitized strings (e.g. in GetMinAndMaxTimestampForTraceID). Use parameterized queries instead to mitigate SQL injection risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/app/clickhouseReader/reader.go:5218
- Draft comment:
Avoid reusing the same variable name (e.g. 'query') in one function; this shadows earlier values (in ReadRuleStateHistoryByRuleID). Use distinct names for different queries to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pkg/query-service/app/clickhouseReader/reader.go:5648
- Draft comment:
Typo detected: variable name 'atrributeValue' should be 'attributeValue' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/app/clickhouseReader/reader.go:4240
- Draft comment:
The reflection‐based logic in readRow is intricate. Consider refactoring or adding comments to clarify the handling of pointer and double‐pointer types for numeric values. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/app/clickhouseReader/reader.go:5620
- Draft comment:
Many SQL queries are built using fmt.Sprintf with inline concatenation. Consider centralizing query construction (or mapping table names) and using fully parameterized queries where possible to both improve readability and security. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/app/clickhouseReader/reader.go:1888
- Draft comment:
TTL‐setting functions contain duplicated logic (e.g. in SetTTLLogsV2, SetTTLTracesV2, and SetTTL). Refactor common TTL code into helper functions to reduce duplication and improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pkg/query-service/app/clickhouseReader/reader.go:4387
- Draft comment:
The dynamic allocation via reflection in GetTimeSeriesResultV3 is brittle. Ensure that all column ScanTypes are supported, and consider adding unit tests for various column types. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pkg/query-service/app/clickhouseReader/reader.go:5975
- Draft comment:
When building similarity scores in GetRelatedMetrics, the heuristic (weighted_match_count + raw_match_count/10) is hardcoded. Consider making this configurable or adding more documentation to explain the rationale. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
14. pkg/query-service/app/clickhouseReader/reader.go:6070
- Draft comment:
For queries that join user inputs into IN clauses (e.g. in GetMetricsDataPointsAndLastReceived), verify that inputs are properly sanitized or consider using parameterized IN queries if supported. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. pkg/query-service/app/clickhouseReader/reader.go:6400
- Draft comment:
The overall file is extremely large. Consider splitting the ClickHouseReader implementation into multiple files or packages (e.g. separating metrics, logs, traces, and TTL functions) to improve maintainability and ease of navigation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_pO3mBNYNlCgBD8Cg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Related metrics api
Related Issues / PR's
Important
Introduces a new API endpoint to fetch related metrics based on name and attribute similarity, with changes across database queries, API handlers, and data models.
GetRelatedMetrics
toreader.go
for fetching related metrics based on name and attribute similarity.GetRelatedMetrics
inSummaryService
insummary.go
to process and return related metrics.ParseRelatedMetricsParams
inparser.go
for parsing related metrics request parameters./api/v1/metrics/related
endpoint inhttp_handler.go
to expose related metrics functionality.RelatedMetricsRequest
,RelatedMetricsResponse
, andRelatedMetricsScore
tometrics_explorer/summary.go
.Reader
interface ininterfaces.go
to includeGetRelatedMetrics
.NormalizeMap
informat.go
for normalizing metric scores.This description was created by
for ead25be. It will automatically update as commits are pushed.