-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Extend Metrics Transform to be able to add a label to an existing metric #441
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,6 +94,12 @@ func validateConfiguration(config *Config) error { | |
if op.Action == UpdateLabel && op.Label == "" { | ||
return fmt.Errorf("missing required field %q while %q is %v in the %vth operation", LabelFieldName, ActionFieldName, UpdateLabel, i) | ||
} | ||
if op.Action == AddLabel && op.NewLabel == "" { | ||
return fmt.Errorf("missing required field %q while %q is %v in the %vth operation", NewLabelFieldName, ActionFieldName, AddLabel, i) | ||
} | ||
if op.Action == AddLabel && op.NewValue == "" { | ||
return fmt.Errorf("missing required field %q while %q is %v in the %vth operation", NewValueFieldName, ActionFieldName, AddLabel, i) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ Duplicated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite duplicated but I can condense it to say that both NewLabel/NewValue are required. It would make the error message less precise as it won't say what is missing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad 🤦♂️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries :) |
||
} | ||
} | ||
|
||
|
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
NewValue
field here used to mean the new label value for the label values being aggregated together, but it seems like in this actionadd_label
, this field means the description for the new label. Please correct me if I am wrong, but this seems like a pretty big difference for this one field in two different actions. Maybe consider having a new field callednewDescription
or something?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.
Side note: we probably need to break this struct up for different operation types at some point.
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.
This is just an error in setting the label values at the metric descriptor and not at the time series. Since the intent of hte pr is to update the label values they have the same goal (either in add a new label or when it gets aggregated). To make it cleared, i'll update the comment to say