feat: Sumo Logic multi-query (ABC pattern) support [PC-18992]#889
feat: Sumo Logic multi-query (ABC pattern) support [PC-18992]#889nikodemrafalski wants to merge 3 commits intomainfrom
Conversation
Fix FormatQuery nil return for multi-query metrics by using GetQueries() to return the last query. Add nil guard for Query pointer dereference in count metrics timeslice validation. Remove unused sumoLogicValidRowIDs variable. Add unit tests for GetQueries() and multi-query count metrics.
There was a problem hiding this comment.
Pull request overview
This PR adds multi-query (ABC pattern) support for Sumo Logic metrics, enabling up to 6 query rows (A–F) where later rows can reference earlier ones using #A, #B, etc. The change maintains backward compatibility with the existing single Query field while introducing a new Queries slice field.
Changes:
- New
SumoLogicQuerystruct andQueriesfield onSumoLogicMetric, with aGetQueries()helper that normalizes both legacy and new representations into a single code path. - Comprehensive validation: mutual exclusivity of
query/queries, row ID uniqueness and range (A–F), slice length (1–6), queries forbidden for logs type, and count metrics quantization matching updated for both paths. - Updated
FormatQueryto useGetQueries()for the SumoLogic case, returning the last query row's content.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
manifest/v1alpha/slo/metrics_sumo_logic.go |
New SumoLogicQuery struct, Queries field, GetQueries() method, multi-query validation rules, null safety guard in count metrics validation |
manifest/v1alpha/slo/metrics_sumo_logic_test.go |
Tests for multi-query validation, GetQueries() normalization, count metrics with multi-query, and updated error expectations |
manifest/v1alpha/slo/metrics.go |
Updated FormatQuery to use GetQueries() and return the last query row for SumoLogic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
skrolikiewicz
left a comment
There was a problem hiding this comment.
PR Review
Existing comments (Copilot)
Both are valid:
- Dead code in
FormatQuery— thereturn m.SumoLogic.Queryfallback is unreachable sinceGetQueries()already normalizes the legacy field. - Extra blank line at
metrics_sumo_logic.go:187— trivial formatting.
New Findings
Breaking change: query field gained omitempty — Medium-High
The JSON tag changed from json:"query" to json:"query,omitempty". Previously, serializing a SumoLogicMetric with Query: nil produced "query": null in JSON. Now it omits the key entirely. Any downstream consumer expecting the query key to always be present will break. This is a backward-incompatible serialization change that should be intentional and documented, or reverted.
FormatQuery "last query" semantics — Low
queries[len(queries)-1].Query returns the last row (e.g. #A - #B), which is typically the "result" expression. This is reasonable but the rationale isn't documented. No external callers were found in this repo, so low risk.
Everything else looks correct
- Mutual exclusivity of
query/queriesis properly enforced - Row ID validation (A-F, unique) is solid
queriesis correctly forbidden for logs type- Count metrics quantization comparison handles
Query == nilcorrectly (the nil guard added in the fix commit) GetQueries()normalization logic is clean and well-tested- Test coverage is thorough across all validation paths
|
/build |
skrolikiewicz
left a comment
There was a problem hiding this comment.
PR Review (Round 2)
All three issues from the previous review have been addressed in f97f239:
Dead code in— fixed, now uses clean early-return patternFormatQuery— reverted toqueryomitempty breaking changejson:"query"Extra blank line— removed
Fresh review of the current state found no remaining issues. Validation logic is correct across all paths, nil guards are properly placed, JSON tags preserve backward compatibility, and test coverage is thorough.
LGTM
Motivation
Sumo Logic metrics support a multi-query (ABC) pattern where users define up to 6 query rows (A–F) and later rows reference earlier ones using
#A,#B,#Csyntax (e.g.,#C = (#A / #B) * 100). This enables complex SLI calculations that aren't possible with a single query.Summary
SumoLogicQuerystruct withRowID(A–F) andQueryfieldsQueries []SumoLogicQueryfield onSumoLogicMetric, mutually exclusive with the existingQueryfieldGetQueries()helper normalizes both representations into a single code pathQueryfield marked as deprecated but fully supported for backward compatibilityRelease Notes
SumoLogicMetricnow supports multi-query (ABC pattern). A newQueriesfield accepts up to 6 query rows (A–F), where later rows can reference earlier ones via#A,#B, etc. TheGetQueries()method provides backward-compatible access.QueryandQueriesare mutually exclusive;Queriesis only allowed for the metrics type.PC-18992