Skip to content

Preserve underscores in resource group metric tags - #6553

Open
efegokdemir wants to merge 4 commits into
apache:mainfrom
efegokdemir:codex/6539-resource-group-metrics
Open

efegokdemir wants to merge 4 commits into
apache:mainfrom
efegokdemir:codex/6539-resource-group-metrics

Conversation

@efegokdemir

@efegokdemir efegokdemir commented Sep 23, 2026 •

Copy link
Copy Markdown

Summary

Fixes the monitor's resource-group lookup after Micrometer formats metric tag names.

Changes

  • Retain Micrometer-compatible MetricsUtil.formatString behavior.
  • Resolve a formatted resource-group metric tag against the configured resource-group names in monitor-side processing.
  • Preserve the formatted value when there is no match or when multiple configured names map to the same formatted value.
  • Add regression coverage for underscore preservation and ambiguous matches.

Testing

  • mvn -pl core -am -Dtest=MetricsUtilTest -DskipITs -Dcheckstyle.skip=true test -q — passed.
  • git diff --check — passed.

Notes

The implementation follows the requested matching approach: it keeps canonical formatting and resolves back to a configured resource-group name only when the formatted value has one unique match.

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@dlmarion

Copy link
Copy Markdown
Contributor

Removing the MetricUtil.formatString formatting is likely not the correct approach here as the formatting is consistent with Micrometer's suggested format. Instead, we probably need a method that takes the resource group tag value and the set of valid resource group names and returns the match.

@efegokdemir

Copy link
Copy Markdown
Author

Updated the PR description to match the current implementation. Micrometer-compatible formatting is retained; monitor-side processing now resolves the formatted tag against configured resource-group names and preserves ambiguous/non-matching values. The focused MetricsUtilTest and reactor test command pass, and git diff --check passes. Please re-review the current HEAD when convenient.

}
match = validName;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might make sense to collect all valid matches in Set. After checking all, if the Set size is one, then we have a match. If the Set size is greater than one, then we can't determine the actual match. This covers the case where resource-group and resource_group both get formatted to resource.group. Ideally the user would not do this, but it's possible. If there is no match, or multiple matches, then I would throw an IllegalStateException with an appropriate message.

Comment on lines +875 to +876
String queueName = MetricsUtil.resolveResourceGroupName(t.value(),
configuredCompactionResourceGroups);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should catch the suggested IllegalStateException, log it, and continue to the next tag.

Comment on lines 997 to 999
queueName = MetricsUtil.resolveResourceGroupName(tag.value(),
configuredCompactionResourceGroups);
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should catch the suggested IllegalStateException and log it

Comment on lines +50 to +51
assertEquals("user.small",
MetricsUtil.resolveResourceGroupName("user.small", Set.of("user_small", "user-small")));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this behavior is correct. It's returning a resource group name that is not valid.

@dlmarion dlmarion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this behavior is correct. It's returning a resource group name that is not valid.

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Addressed the current-head feedback in aa8eff8470: resource-group resolution now returns a name only for a unique configured match. Ambiguous or unknown formatted tags are rejected before constructing ResourceGroupId or updating the queued-resource-group map, so the monitor cannot surface a non-configured resource group. Added the unknown-name regression. Validation: mvn -pl core -am -Dtest=MetricsUtilTest -DskipITs -Dcheckstyle.skip=true test -q, monitor compile, and git diff --check passed.

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Updated eae6317 to follow the requested zero/ambiguous-match semantics: MetricsUtil now throws an informative IllegalStateException unless exactly one configured resource group matches. The monitor callers catch it, log the invalid tag, and skip that metric instead of retaining an invalid name. Added/updated focused resolution tests.

Validation: mvn -pl core -am -Dtest=MetricsUtilTest -DskipITs -Dcheckstyle.skip=true test -q passed; git diff --check passed.

This branch has not been deployed

No deployments
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.

2 participants