chore: add regex capture to label application rule #214
Conversation
| if (tokenExtractionRule.hasAlias()) { | ||
| validKeys.add(tokenExtractionRule.getAlias()); | ||
| } else { |
There was a problem hiding this comment.
Unintentional change?
There was a problem hiding this comment.
Nope. If alias is provided then alias should be used not key.
There was a problem hiding this comment.
Okay. So, till now we were validating key in case of alias as well, even though it wasn't getting used later?
There was a problem hiding this comment.
Earlier both alias and key were being used. I will change to use alias if alias is provided.
Please note this expression is not exposed in UI so far, so no one should be using it.
| if (tokenExtractionRule.hasAlias()) { | ||
| validKeys.add(tokenExtractionRule.getAlias()); | ||
| } else { |
There was a problem hiding this comment.
Okay. So, till now we were validating key in case of alias as well, even though it wasn't getting used later?
| @@ -264,6 +266,15 @@ public void validateLabelExpression(Action.DynamicLabel dynamicLabel) { | |||
| private void validateTokenExtractionRule( | |||
| Action.DynamicLabel.TokenExtractionRule tokenExtractionRule) { | |||
| validateNonDefaultPresenceOrThrow(tokenExtractionRule, tokenExtractionRule.KEY_FIELD_NUMBER); | |||
| if (tokenExtractionRule.hasRegexCapture()) { | |||
| try { | |||
| Pattern.compile(tokenExtractionRule.getRegexCapture()); | |||
There was a problem hiding this comment.
I think we have a validator somewhere that validates a number of capture groups - and we should be using re2
There was a problem hiding this comment.
Oh we are :) I forget they use identical names to make it drop in. Anyway, RegexValidator.validateCaptureGroupCount is what I was referring to
| @@ -264,6 +265,9 @@ public void validateLabelExpression(Action.DynamicLabel dynamicLabel) { | |||
| private void validateTokenExtractionRule( | |||
| Action.DynamicLabel.TokenExtractionRule tokenExtractionRule) { | |||
| validateNonDefaultPresenceOrThrow(tokenExtractionRule, tokenExtractionRule.KEY_FIELD_NUMBER); | |||
| if (tokenExtractionRule.hasRegexCapture()) { | |||
| RegexValidator.validateCaptureGroupCount(tokenExtractionRule.getRegexCapture(), 1); | |||
There was a problem hiding this comment.
Why are we restricting only to one capture group?
There was a problem hiding this comment.
Since we need a single value, we can use value only from one group
There was a problem hiding this comment.
Oh, I was thinking of allowing users to use the values of each group accordingly if they want to like we have in Naming rules. for example: if the attribute value like: domain.sub-domain1.sub-domain2 and we want to extract: domain:sub-domain2 for label, then we need to support multiple capture groups.
There was a problem hiding this comment.
You'd create create two rules with one capture group each.
There was a problem hiding this comment.
But that would lead to two different labels, right? If we want both of of them in a single label?
There was a problem hiding this comment.
No - a dynamic label defines tokens and an expression that indicates how to combine them - this PR is adding a new way to extract a token:
message DynamicLabel {
string label_expression = 1; // expression to combine tokens to generate label, example "${deployment}_${version}"
repeated TokenExtractionRule token_extraction_rules = 2;
message TokenExtractionRule {
string key = 1;
optional string json_path = 2;
optional string alias = 3;
optional string regex_capture = 4;
}
}
Defined regex capture to perform operation on attribute value like substring etc.