search

eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

Fixes GaugeInjectionBeanTest setup #563

Closed jbee closed 4 years ago

jbee commented 4 years ago

The idea of using @Before to make sure the gauge gets registered before the test is running is flawed in the sense that the test is only successful if the gauge is successfully injected into the instance of the test class. This happens before @Test and @Before run so at that point the gauge does not exist should the gauge field be injected first. Or maybe even if it is injected after bean it might not exist due to proxies.

I have not tested the fix of using @BeforeClass but I'd suspect this should force the usage of bean before the test class instance is injected and thereby making sure the gauge exists so it is successfully injected into the test class instance.

Another idea to fix this is to use a empty test method an annotate it with @InSequence(1) and make the real tests 2 and 3.

Signed-off-by: Jan Bernitt jaanbernitt@gmail.com

eclipse-microprofile-bot commented 4 years ago

Can one of the admins verify this patch?

donbourne commented 4 years ago

ok to test

Channyboy commented 4 years ago

This is causing failure when running a managed/remote arquilllian set-up. The BeforeClass runs in the client side and can't access the CDI API.

jmartisk commented 4 years ago

This looks like a bit of an issue in the spec, because it might be that a gauge is injected before it is actually registered (because gauges are registered lazily - when their owning bean is instantiated, which might be at an unpredictable point in time).

In SmallRye we solve this by injecting a sort of proxy gauge, which, when its getValue() is called, performs a lookup in the registry to find the actual gauge, and calls it to get the actual value. And if that gauge still does not exist at the time when getValue() is called on the injected proxy, the call to the proxy returns null.

I think the spec does not explain what should correctly happen in such case. Perhaps we could make this SmallRye behavior the "correct" one? I don't think just injecting a gauge should somehow "force" it to register, that would mean initializing the owning CDI bean, which could be unexpected fiddling with the bean's lifecycle.

jbee commented 4 years ago

I'd be happy with making the proxy the official solution. Although it is somewhat inconsistent with rest of metrics where a missing metric causes exceptions. Gauges would no longer have that and instead just fail "silently".

jbee commented 4 years ago

@Channyboy Thanks for testing it. I changed this PR to use the work-around with a dummy test that runs first. This should process the bean and make sure the actual tests that run as 2 and 3 work with gauge already present. Again I cannot really test this as I have no compliant implementation yet but I'd assume this should solve the issue.

jmartisk commented 4 years ago

Although it is somewhat inconsistent with rest of metrics where a missing metric causes exceptions. Gauges would no longer have that and instead just fail "silently".

I don't think it's inconsistent. If you @Inject different metric types, it leads to registering that metric if it's not registered yet. The only difference is that we can't "force register" a gauge because its lifecycle depends on the owning bean, so instead of forcing a registration, we just wait until it is registered elsewhere.

jbee commented 4 years ago

@jmartisk makes sense. Makes me even more in favour of making the proxy the official solution.

jmartisk commented 4 years ago

@donbourne WDYT about this behavior specification?

I suppose if we went that way, this PR would not be needed because a correct implementation would pass test as it is now.

donbourne commented 4 years ago

The behavior suggested makes sense to me. Using a proxy seems to be the most obvious way to handle this as it ensures we can satisfy the needs for CDI injection before a CDI bean with the target gauge is first used.

Is there anything we would want to say in the spec about this, or should we just leave this to implementers to figure out?

jmartisk commented 4 years ago

Ha, it seems the spec currently says that you can't do @Inject @Metric on gauges at all (https://github.com/eclipse/microprofile-metrics/blob/3.0-M2/spec/src/main/asciidoc/app-programming-model.adoc#field)

@Metric can only be used on injected fields of type Meter, Timer, Counter, and Histogram.

Obviously that's at odds with what the TCK verifies. I'll try to come up with an update to that. This also misses SimpleTimer.

jmartisk commented 4 years ago

I submitted https://github.com/eclipse/microprofile-metrics/pull/571 - AFAIU if accepted, it will replace this PR.

jmartisk commented 4 years ago

This was replaced by #571