org.eclipse.microprofile.metrics.tck.tags.GlobalTagsTest#doWithConfig is wrong

[rmannibucau]

this impl registers the config for the TCCL but then MetricsID uses get(null) so both can not match in some environment. Either MetricsID should use TCCL explicitly or the test should also register null (or be written differently maybe?)

[jmartisk]

MetricID constructor calls ConfigProvider.getConfig() (https://github.com/eclipse/microprofile-metrics/blob/2.2.1/api/src/main/java/org/eclipse/microprofile/metrics/MetricID.java#L119) which according to the Config spec, should return the Config for the TCCL. So if the Config implementation is correct, that Config should match the one registered in doWithConfig. Or am I missing something?

[rmannibucau]

Is "No but yes" a valid answer ;)?

More seriously you are right but it was not correctly defined and implemented in early config versions of the spec/impl, and it is not even an impl option in some osgi env. Therefore any impl relying on that and usable with any config impl should take that into account (another reason there shouldnt be any impl in mp api jars).

It is just about tck so we can always patch the "env" but it is yet another reason to drop any impl from api jar so think it is worth tracking.

[jmartisk]

This test has always been kind of problematic and I've been considering removing it multiple times, as maybe it creates more problems than value. I'd like to rewrite it to be less reliant on MP Config implementation, but I don't know how to test this without creating a new Config instance (and dumping the old one), that's probably not possible. Therefore we welcome concrete suggestions how to improve it :) if it turns out to be too problematic, I would not be much opposed to removing it, but AFAIK it should now be correct with respect to MP Config behavior.

I totally agree with minimizing the impl work done in the API code and reported an issue so we can have a deep look for the future releases, but won't be able to do this for 2.3, which is due very soon.

[rmannibucau]

The test itself can provide a properties file in the a war which will be taken in the context of the test only I think. Can be saner than hacking config instance. Wdyt?

[jmartisk]

The problem with that is that properties in META-INF/microprofile-config.properties have lower priority than system properties or environment variables (https://github.com/eclipse/microprofile-config/blob/master/spec/src/main/asciidoc/configsources.asciidoc#default-configsources) so they will not be taken into account, because the rest of the test suite expects a different value of the property (mp.metrics.tags) specified as an environment variable.

[rmannibucau]

@jmartisk why? :angel:

Concretely the TCK suite can provide an archive appender (or processor) to add it each time it is needed, it would even enable to test without the env variable in the same suite. Don't think it is a technical blocker. Am I missing something?

[jmartisk]

Hmmm yes, perhaps it would work if we stopped requiring mp.metrics.tags being passed globally while starting the tests, and instead it would get added into the archive using Shrinkwrap for each test that needs it. @donbourne would you agree with such change? I think it makes sense. Requiring a particular variable to be passed globally is a stupid idea in general.

[donbourne]

I think what you're proposing is that we stop testing the case where mp.metrics.tags is provided using an env var, and instead test with the config supplied META-INF/microprofile-config.properties file. While I'd be a bit sad to see the env var test disappear, testing using the file-specified properties would better isolate the tests, so I think I would be in favor of that. @Channyboy , what do you think?

Also...

More seriously you are right but it was not correctly defined and implemented in early config versions of the spec/impl, and it is not even an impl option in some osgi env.

...Are you implying there that the config spec is requiring something that can't be universally implemented? That seems to be at the root of this issue.

[rmannibucau]

File, custom source, system prop or env var, all that is under config spec and already tested so no reason to duplicate it technically.

[donbourne]

@rmannibucau that's a good point.. we don't need to test that config is working as expected, just that the config when supplied to Metrics works as expected. So specifying the config in a file is just as good as specifying it in an env var from that perspective.