Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Put metric before label value in error message when 'label value too long' #3982

Closed
wants to merge 2 commits into from
Closed

Put metric before label value in error message when 'label value too long' #3982

wants to merge 2 commits into from

Conversation

ChinYing-Li
Copy link
Contributor

(#1582)

Signed-off-by: ChinYing-Li [email protected]

What this PR does:
Swap the position of the metric string and the label value string in error message of labelValueTooLong.
Now the metric string is placed before the label value string.

Which issue(s) this PR fixes:
Fixes #1582

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@ChinYing-Li ChinYing-Li force-pushed the issue_1582 branch 2 times, most recently from 9697e9b to 25bbfed Compare March 20, 2021 21:08
@pstibrany pstibrany mentioned this pull request Mar 22, 2021
Merged
3 tasks
pracucci
pracucci reviewed Mar 22, 2021
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! LGTM (modulo a couple of nits)

CHANGELOG.md Outdated
@@ -10,6 +10,7 @@
* `-alertmanager.cluster.advertise-address` instead of `-cluster.advertise-address`
* `-alertmanager.cluster.peers` instead of `-cluster.peer`
* `-alertmanager.cluster.peer-timeout` instead of `-cluster.peer-timeout`
* [CHANGE] Put metric before label value in error message when `labelValueTooLong`. #3982
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* [CHANGE] Put metric before label value in error message when `labelValueTooLong`. #3982
* [ENHANCEMENT] Put metric before label value in the "label value too long" error message. #3982

pkg/util/validation/validate.go Outdated
@@ -35,7 +35,7 @@ const (
errInvalidMetricName = "sample invalid metric name: %.200q"
errInvalidLabel = "sample invalid label: %.200q metric %.200q"
errLabelNameTooLong = "label name too long: %.200q metric %.200q"
errLabelValueTooLong = "label value too long: %.200q metric %.200q"
errLabelValueTooLong = "label value too long: metric %.200q value %.200q"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errLabelValueTooLong = "label value too long: metric %.200q value %.200q"
errLabelValueTooLong = "label value too long for metric %.200q label name: %.200q"

@pracucci
Copy link
Contributor

@ChinYing-Li Sorry to bother you. I've just merged #3990 which impacts on this PR. Could you apply the same change (picking up the suggestion I gave above) to the error which is now defined in pkg/util/validation/errors.go, please?

@ChinYing-Li
Copy link
Contributor Author

Given the refactor, now there's no simple way to identify whether s genericValidationError is a label value too long error, hence unable to make label value too long a special case in func (e *genericValidationError) Error() string. Introducing another member e.g. errorType to genericValidationError would be against the design; do you have any suggestion on how to approach this? @pracucci

@pstibrany
Copy link
Contributor

Given the refactor, now there's no simple way to identify whether s genericValidationError is a label value too long error, hence unable to make label value too long a special case in func (e *genericValidationError) Error() string. Introducing another member e.g. errorType to genericValidationError would be against the design; do you have any suggestion on how to approach this? @pracucci

I think the solution is to not use genericValidationError for "label value too long", but define custom error.

@pracucci
Copy link
Contributor

I think the solution is to not use genericValidationError for "label value too long", but define custom error.

Yes, I agree.

@ChinYing-Li ChinYing-Li mentioned this pull request Mar 26, 2021
Merged
3 tasks
@ChinYing-Li
Copy link
Contributor Author

Closing this PR. Relevant development done based on recent changes is at #4018.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci left review comments

Assignees
No one assigned
Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error message for 'label value too long' misses important information
3 participants
@ChinYing-Li @pracucci @pstibrany