Skip to content

Export ValidateMatcher for DI (#2)#2716

Merged
roidelapluie merged 1 commit intoprometheus:mainfrom
grafana:matcherValidation2
Oct 21, 2021
Merged

Export ValidateMatcher for DI (#2)#2716
roidelapluie merged 1 commit intoprometheus:mainfrom
grafana:matcherValidation2

Conversation

@kylebrandt
Copy link
Contributor

So third parties, Grafana in particular, can override the validation.

Grafana wants to do this because other data sources will have label keys with things like spaces, periods, or other characters - and looking for a better integration with alert manager.

goes with grafana/grafana#38629
replaces #2694 with more minimal changes, as most of that was able to be moved into Grafana's code base.

so third parties, Grafana in particular, can over ride the validation.

Grafana wants to do this because other data sources will have label keys with things like spaces, periods, or other characters - and looking for a better integration with alert manager.

goes with grafana/grafana#38629
replaces #2694

Signed-off-by: Kyle Brandt <kyle@grafana.com>
Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

@roidelapluie
Copy link
Member

is that the only way to do this? exporting a function as global variable is not that great.

@kylebrandt
Copy link
Contributor Author

is that the only way to do this? exporting a function as global variable is not that great.

Perhaps it come be done so ValidateMatcherFunc func(m *pb.Matcher) error is added to the Options struct. Then it could be passed as private property on the Silences struct, and validateSilence could take the function as an additional argument. If it is nil call the builtin validateMatcher, if not nil call what was passed.

Does that sound better?

Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

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

Thanks, note that if you allow dots, you should not allow ... as label name because it's special in alertmanager

@roidelapluie roidelapluie merged commit 1b8afe7 into prometheus:main Oct 21, 2021
markoposavec pushed a commit to markoposavec/alertmanager that referenced this pull request Nov 4, 2021
so third parties, Grafana in particular, can over ride the validation.

Grafana wants to do this because other data sources will have label keys with things like spaces, periods, or other characters - and looking for a better integration with alert manager.

goes with grafana/grafana#38629
replaces prometheus#2694

Signed-off-by: Kyle Brandt <kyle@grafana.com>
markoposavec pushed a commit to markoposavec/alertmanager that referenced this pull request Nov 4, 2021
so third parties, Grafana in particular, can over ride the validation.

Grafana wants to do this because other data sources will have label keys with things like spaces, periods, or other characters - and looking for a better integration with alert manager.

goes with grafana/grafana#38629
replaces prometheus#2694

Signed-off-by: Kyle Brandt <kyle@grafana.com>
Signed-off-by: Marko Posavec <Marko.Posavec@infobip.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants