Measure labels and formatting in alert emails #4731
Merged
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
esevastyanov
changed the title
begelundmuller
requested changes
Apr 28, 2024
runtime/queries/proto.go
Outdated
Comment on lines
64
to
65
| // ProtoFromJSON builds a proto query from a query name, JSON args, and optional execution time. | ||
| func ProtoFromJSON(qryName, qryArgsJSON string, executionTime *time.Time) (*runtimev1.Query, error) { | ||
| func ProtoFromJSON(qryName, qryArgsJSON string, executionTime *time.Time) (*runtimev1.Query, string, error) { |
There was a problem hiding this comment.
Can we split this out into a func MetricsViewFromQuery(qry *runtimev1.Query) (name string, ok bool) function instead? It would need to reproduce the switch, but would nicely separate the concerns (e.g. in case we need to use ProtoFromJSON for non-metrics queries in the future.)
runtime/reconcilers/alert.go
Outdated
Comment on lines
895
to
917
| m := row.AsMap() | ||
| res := make(map[string]any) | ||
| for k, v := range m { | ||
| var measureLabel string | ||
| var f formatter.Formatter | ||
| for _, measure := range measures { | ||
| if k == measure.Name { | ||
| measureLabel = measure.Label | ||
| var err error | ||
| // D3 formatting isn't implemented yet so using the format preset for now | ||
| f, err = formatter.NewPresetFormatter(measure.FormatPreset, false) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| break | ||
| } | ||
| } | ||
| if measureLabel == "" { | ||
| measureLabel = k | ||
| } | ||
| res[measureLabel] = formatValue(f, v, logger) | ||
| } | ||
| return res, true, nil |
There was a problem hiding this comment.
- Can we pull this out into a util function to avoid too much nesting? E.g.
func formatResultRow(row map[string]any, metricsView) map[string]any - Do you think it would make sense to log the error from
NewPresetFormatterand output the non-formatted values? Instead of failing the alert due to an error in the format preset.
runtime/reconcilers/alert.go
Outdated
Comment on lines
926
to
929
| var measureLabel string | ||
| var f formatter.Formatter | ||
| for _, measure := range measures { | ||
| if v.MeasureName == measure.Name { |
There was a problem hiding this comment.
Same comments as above about a util func (probably a different one since the logic is quite different) and maybe falling back to non-formatted measures instead of failing
k-anshul
pushed a commit
that referenced
this pull request
Apr 30, 2024
* Measure value formatter * Turned most structures private * Formatting in alerts * Revert "Formatting in alerts" This reverts commit 0aa937b. * Fixed code style * Alert number formatting * Simplification * Split a function and reduced nesting level * Fixed nil dereference issue * Reduced nesting level
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.
No description provided.