feat(summary): added alerts and basic query updates #7174
+158
−48
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.
❌ Changes requested. Reviewed everything up to 123a308 in 3 minutes and 23 seconds
More details
- Looked at
341lines of code in6files - Skipped
0files when reviewing. - Skipped posting
18drafted comments based on config settings.
1. pkg/query-service/rules/manager.go:56
- Draft comment:
The constant taskNamesuffix is defined as "webAppEditor" but task names are later constructed using "-groupname". Consider using a single constant for 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.
2. pkg/query-service/rules/manager.go:59
- Draft comment:
RuleIdFromTaskName splits on "-groupname". Ensure this splitting logic aligns with the task naming convention across the code. - 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/rules/manager.go:393
- Draft comment:
In editTask, after stopping the old task, newTask.CopyState(oldTask) is used. Confirm that stopping and copying state are both thread-safe to avoid race conditions. - 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/rules/manager.go:811
- Draft comment:
TestNotification calls prepareTestRuleFunc; ensure this function is set (or add a nil check) to prevent potential panics. - 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/rules/manager.go:866
- Draft comment:
The equality check for metric names in GetAlertDetailsForMetricNames is case sensitive. Consider whether a case-insensitive comparison might be more appropriate based on naming conventions. - 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 case-sensitivity in metric names could potentially cause issues, there's not enough context to know if this is actually a problem. Metric names may be expected to be case-sensitive. Without evidence that case-insensitive comparison is needed or documentation of naming conventions, this feels speculative. The comment also starts with "Consider whether..." which makes it more of a question than a clear issue.
The suggestion could prevent bugs where metrics are missed due to case mismatches. Case-sensitivity in identifiers is a common source of confusion.
Without clear evidence that case-insensitive comparison is needed or documentation of metric naming conventions, this comment is speculative and could lead to unnecessary changes. The current behavior may be intentional.
Delete the comment as it is speculative and asks the author to "consider" something without clear evidence of an actual issue.
6. pkg/query-service/rules/manager.go:772
- Draft comment:
In PatchRule, if writing to the DB fails, the task state is rolled back by syncing with the stored rule. Ensure this rollback mechanism is robust to avoid lingering inconsistent states. - 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/rules/manager.go:495
- Draft comment:
Improve error messages like 'error loading rules, previous rule set restored' to include more contextual details for easier debugging. - 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/rules/manager.go:520
- Draft comment:
The use of the m.block channel to delay task execution until the manager is running is subtle. Please add more documentation/comments and ensure it is closed exactly once (e.g. in run()) to avoid potential race or deadlock issues. - 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/rules/manager.go:760
- Draft comment:
In PatchRule, when unmarshalling the stored rule data and applying the patch, consider wrapping errors with more context to ease debugging in case of failures. - 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/rules/manager.go:600
- Draft comment:
Throughout the Manager code, zap.L() is used directly. For better testability and configuration consistency, consider using the logger instance provided in ManagerOptions (m.logger) everywhere. - 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/rules/manager.go:483
- Draft comment:
The task management logic in addTask, editTask, and deleteTask is quite similar. Consider refactoring common 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:3899
- Draft comment:
Typographical error: use 'at least' instead of 'atleast' in the error message when checking the length of metricNames (e.g. line 3899). - 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:3190
- Draft comment:
Typographical issue in the SQL query within UpdateLogField: the clause for adding the materialized column uses a closing backtick () after %s_exists but is missing its opening backtick. It should be formatted as%s_exists` to ensure proper quoting of the column name. - 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.
14. pkg/query-service/app/clickhouseReader/reader.go:2868
- Draft comment:
Minor typographical improvement: consider renaming 'GetSpansInLastHeartBeatInterval' to 'GetSpansInLastHeartbeatInterval' to follow standard spelling. - 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/metricsexplorer/summary.go:214
- Draft comment:
Typographical error detected: The constant 'metrics_explorer.TimeSeriesTeeMap' at line 214 seems like it might be a typo. Consider revising it to 'TimeSeriesTreeMap' if that was the intended naming. - 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.
16. pkg/query-service/model/metrics_explorer/summary.go:19
- Draft comment:
It appears that 'TimeSeriesTeeMap' might be a typographical error. Perhaps it was intended to be 'TimeSeriesTreeMap' for consistency with 'SamplesTreeMap'. Please review and update if needed. - 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.
17. pkg/query-service/rules/manager.go:172
- Draft comment:
Typo: In the comment '// create ch rule task for evalution', 'evalution' should be corrected to 'evaluation'. - 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.
18. pkg/query-service/rules/manager.go:192
- Draft comment:
Typo: In the comment '// create promql rule task for evalution', 'evalution' should be corrected to 'evaluation'. - 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_IcpsSG5ZeM653WUj
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.
| func (m *Manager) GetAlertDetailsForMetricNames(ctx context.Context, metricNames []string) (map[string][]GettableRule, error) { | ||
| result := make(map[string][]GettableRule) | ||
|
|
||
| rules, err := m.ruleDB.GetStoredRules(ctx) |
There was a problem hiding this comment.
In GetAlertDetailsForMetricNames, all stored rules are fetched and then filtered in memory. For scalability, consider pushing the AlertType filter (and other conditions) into the DB query so that only metric alerts are returned.
srikanthccv
reviewed
Feb 23, 2025
|
|
||
| sb.WriteString(fmt.Sprintf(" LIMIT %d OFFSET %d;", req.Limit, req.Offset)) | ||
|
|
||
| sampleQuery := sb.String() |
There was a problem hiding this comment.
We should do this in GetMetricsSamplesPercentage as well
| reader interfaces.Reader | ||
| querierV2 interfaces.Querier | ||
| reader interfaces.Reader | ||
| alertManager *rules.Manager |
Comment on lines
+866
to
+883
| for _, query := range rule.RuleCondition.CompositeQuery.BuilderQueries { | ||
| if query.AggregateAttribute.Key == "" { | ||
| continue | ||
| } | ||
|
|
||
| // Check if this metric name is in the requested list | ||
| for _, metricName := range metricNames { | ||
| if query.AggregateAttribute.Key == metricName { | ||
| // Initialize slice if this is first alert for this metric | ||
| if result[metricName] == nil { | ||
| result[metricName] = make([]GettableRule, 0) | ||
| } | ||
| result[metricName] = append(result[metricName], rule) | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This should handle CH queries and promql queries as well. The queryType value contains the correct type of query used. It can be builder, clickhouse_sql, or promql.
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 alerts, segregated last received and data points function for summary view, break the query into optional subquery for list metrics without where clause.
Important
Added alert details to metric summaries, separated data point and last received queries, and updated query handling for optional subqueries.
GetMetricsSummary()insummary.gousingGetAlertDetailsForMetricNames()fromrules/manager.go.GetMetricsDataPointsAndLastReceived()intoGetMetricsDataPoints()andGetMetricsLastReceived()inreader.go.ListSummaryMetrics()inreader.goto handle optional subqueries based onwhereClause.Readerinterface ininterface.goto includeGetMetricsDataPoints()andGetMetricsLastReceived().LastReceivedtype fromuint64toint64inMetricDetailsDTOinsummary.go.NewSummaryService()insummary.goto acceptalertManagerinstead ofquerierV2.manager.go.This description was created by
for 123a308. It will automatically update as commits are pushed.