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

How is mp.metrics.appName supposed to work? #216

Open
jmesnil opened this issue Dec 2, 2019 · 6 comments
Open

How is mp.metrics.appName supposed to work? #216

jmesnil opened this issue Dec 2, 2019 · 6 comments

Comments

@jmesnil
Copy link
Contributor

jmesnil commented Dec 2, 2019

MP Metrics provides a recommendation to support multiple applications
However it also mention that this recommendation also applies to EAR with subdeployments.

It write that the subdeployments can use their META-INF/microprofile-config.properties properties file to specify an unique mp.metrics.appName property.

This does not work with smallrye-metrics. All injected metrics (regardless of the subdeployments) are registered from io.smallrye.metrics.setup.MetricCdiInjectionExtension.
The code will end up in org.eclipse.microprofile.metrics.MetricID#MetricID(java.lang.String, org.eclipse.microprofile.metrics.Tag...) that will call ConfigProvider.getConfig() to load the Config object to read the configuration properties.

At this point, the Config will be fetched corresponding to the TCCL which is the EAR ClassLoader.
All registered metrics in the EAR will use the same Config object and will not use their properties from their respective META-INF/microprofile-config.properties file.

Am I missing something on how this feature is supposed to work for EAP deployment with subdeployments?

@jmartisk
Copy link
Member

Sorry I was on paternity leave, hence the late response.
Do you mean that in WildFly, all modules of an EAR share a common TCCL and a common Config instance? I thought every module had its own TCCL and therefore ConfigProvider.getConfig() would resolve to a separate Config instance scoped to that module. So each module will get its mp.metrics.appName property attached in the config's SubsystemDeploymentProcessor (as in my crude PoC in jmartisk/wildfly@0dd89cf) and later when metrics are being registered, the TCCL will be the TCCL of the particular module, so MetricID constructor will pick up the correct Config instance. Is that not so?

@jmartisk
Copy link
Member

From my quick experiments, it looks like each module has its own Config instance and its own TCCL, and the top-level EAR has got the same as well, but when metrics are being registered by MetricCdiInjectionExtension, the TCCL is set to the TCCL of the whole EAR, not the module. That seems to be the cause why all metrics see the same mp.metrics.appName property. Isn't that a bug, shouldn't the TCCL be the TCCL of the module when CDI extensions are called?

@jmesnil
Copy link
Contributor Author

jmesnil commented Dec 11, 2019

Most importantly, congrats for the baby! :)

I debugged the loading and execution of the MetricCdiInjectionExtension extension in WildFly.

The class is loaded with the EAR module class loader.
When its @observes method are called from org.jboss.as.weld.WeldStartService#start the TCCL is set to the EAR module class loader.

That means that hen metrics are registered in io.smallrye.metrics.setup.MetricCdiInjectionExtension#registerMetrics, the TCCL is still set to the EAR module class loader.
At this point, all metrics uses the same class loader (which is the EAR module class loader) regardless of the actual location of the metrics annotations (in the EAR itself or in subdeployments).

@jmartisk
Copy link
Member

Thanks! :)

Well yep, so the @Observes methods are registered with the TCCL of the top-level deployment, which I think is weird, because each sub-deployment has its own TCCL, so my expectation would be that they should be called with the TCCL of the sub-deployment. I don't know if this is somehow defined by any spec, but intuitively it sounds like a bug in WildFly to me.

Changing this (if it's really a bug) would fix our problem I suppose, because we would then pick up the correct Config which can contain mp.metrics.appName computed for the particular sub-deployment.

@svenhaag
Copy link

The _app feature was removed with MP-Metrics Spec 3.0!
Now, they say, its up to the implementation to support this feature.
I cannot find it yet in SmallRye-Metrics :(.
See: https://download.eclipse.org/microprofile/microprofile-metrics-3.0/microprofile-metrics-spec-3.0.pdf
and: eclipse/microprofile-metrics@6690db4#diff-0eb6f5c712741992073cacdae252851c7dfbdb8d774b4e618c3c259433e39284

@jmartisk
Copy link
Member

SR Metrics itself does not support application-name tags right now, it would be up to the app server (WildFly etc) to provide it, but we haven't figured out how exactly that should be done (SR would probably have to provide some facility to allow it, now that it was moved out of the MP API). Given the status of SmallRye Metrics being deprecated and replaced with Micrometer, I don't see that very likely.
The workaround for now would be to simply make sure you always include an _app (or similar) tag manually in the application's code

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

No branches or pull requests

3 participants