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.

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

Test metrics are registered with correct units #553

Merged
merged 1 commit into from Jun 9, 2020

Conversation

Azquelt
Copy link
Member

@Azquelt Azquelt commented Jun 4, 2020

Fixes #549

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>
@@ -79,6 +87,10 @@ public static WebArchive deploy() {
@Inject
private AllMetricsBean allMetricsBean;

@Inject
@RegistryType(type = Type.BASE)
Copy link
Member

Choose a reason for hiding this comment

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

This made me think:
Do we have test to verify the metrics is under base instead of application?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have an explicit test, but reading the Metrics API docs, I thought that the registries were separate and it wouldn't give you an aggregated view. When you inject a MetricsRegistry without specifying the type, you get the application registry.

In our tests we specify that we want the base registry, both here and in MetricGetter

Copy link
Member Author

@Azquelt Azquelt Jun 5, 2020

Choose a reason for hiding this comment

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

The only other test we could add would be to ensure that metrics are not registered under the application scope. I'm not sure we should add such a test since the spec doesn't explicitly forbid implementers from putting the same metrics in the application scope.

@Azquelt Azquelt merged commit 4d89f07 into eclipse:master Jun 9, 2020
@Azquelt Azquelt deleted the metrics-test-units branch June 9, 2020 14:34
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.

Add TCKs to assert that metrics have the correct type
3 participants