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

chore: add regex capture to label application rule #214

Merged
merged 3 commits into from
Jun 6, 2024
Merged

chore: add regex capture to label application rule #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4707a2e
chore: add regex capture to label application rule
Jun 6, 2024
f98ce9d
add validation
Jun 6, 2024
19f47cd
address review comments
Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...roto/org/hypertrace/label/application/rule/config/service/v1/label_application_rule.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ message LabelApplicationRuleData {
string key = 1;
optional string json_path = 2;
optional string alias = 3;
optional string regex_capture = 4;
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion ...g/hypertrace/label/application/rule/config/service/LabelApplicationRuleValidatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,10 @@ public void validateLabelExpression(Action.DynamicLabel dynamicLabel) {
.getTokenExtractionRulesList()
.forEach(
tokenExtractionRule -> {
validKeys.add(tokenExtractionRule.getKey());
if (tokenExtractionRule.hasAlias()) {
validKeys.add(tokenExtractionRule.getAlias());
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unintentional change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. If alias is provided then alias should be used not key.

Copy link
Contributor

@sanket-mundra sanket-mundra Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay. So, till now we were validating key in case of alias as well, even though it wasn't getting used later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

validKeys.add(tokenExtractionRule.getKey());
}
});
Pattern pattern = Pattern.compile("\\{(\\\\}|[^}])*}");
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we restricting only to one capture group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we need a single value, we can use value only from one group

Copy link
Contributor

@sanket-mundra sanket-mundra Jun 6, 2024 •
edited by aaron-steinfeld
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd create create two rules with one capture group each.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that would lead to two different labels, right? If we want both of of them in a single label?

Copy link
Contributor

Choose a reason for hiding this comment

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

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;
      }
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, okay

}
}

private void throwInvalidArgumentException(String description) {
Expand Down
16 changes: 16 additions & 0 deletions validation-utils/src/main/java/org/hypertrace/config/validation/RegexValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,20 @@ public static Status validate(String regexPattern) {
}
return Status.OK;
}

public static Status validateCaptureGroupCount(String regexPattern, int expectedCount) {
// compiling an invalid regex throws PatternSyntaxException
try {
Pattern pattern = Pattern.compile(regexPattern);
if (pattern.groupCount() != expectedCount) {
return Status.INVALID_ARGUMENT.withDescription(
"Regex group count should be: " + expectedCount);
}
} catch (PatternSyntaxException e) {
return Status.INVALID_ARGUMENT
.withCause(e)
.withDescription("Invalid Regex pattern: " + regexPattern);
}
return Status.OK;
}
}
Loading