Refactor all tests that use Config so that they don't fail locally

[mateuszrzeszutek]

Fixes #1306

There are 3 tests in instrumentation-api module which all set one and the same system property to different values. It seems that during parallel/concurrent test invocation they step on each others toes. Interestingly enough, they seem to work in our CI, probably due to different level of parallelism.

Why did it work on the CI: we use retries on the CI, but not when running tests locally. I suspect that Gradle only re-ran the failing tests (since that would be the sane thing to do), so with each retry NetPeerUtils would be loaded again, but with different property value. And so on until it passed - we use 5 retries (java.gradle) so that should be enough.

I've decided to refactor all tests that modify Config to avoid the same situation in different places:

• for modules that did not need to modify the test configuration I've used the newly-added config SPI to provide static config;
• those that needed to modify the config still use ConfigUtils, but I've refactored it and added Javadocs. I hoped that I could get rid of AgentTestRunner.resetInstrumentation(), but not in this PR unfortunately.

[trask]

Using the new config SPI is nice, but I think will require separate test sets for each different configuration that we want to test with, which feels a little awkward to have to split out test sets for different configuration values.

Also, I kind of like keeping the configuration close to the test that uses it.

And it's nice to have one way to deal with test configuration instead of two.

So I wonder if it's better to use ConfigUtils everywhere for now, until we figure out our desired end state?

One option in the future (which would get rid of resetInstrumentation) is to have Config.getBooleanProperty() return a BooleanConfigValue (etc) and by default BooleanConfigValue can just have a final boolean value in it, but when running tests we could sub out a different impl that always reads from Config.allProperties, so we can update freely at runtime. Instrumentations would have to cache the BooleanConfigValue instead of the raw boolean, but the default impl should perform well since it's just a holder of a constant (non-volatile) value.

[anuraaga]

Agree with @trask that it's probably simpler to stick with the same pattern for static config too. But in general, this looks like a great idea to me, closer to the usual setup/cleanup pattern of saving an old value before tests and restoring it after tests

[anuraaga]

@trask final boolean value - unfortunately, Java won't do anything quite as cool if it's not static XD It's not the end of the world though, avoiding the volatile still helps enough and is a pretty common pattern in Java, just want to point it out. Not sure how much slower resetInstrumentation is since it's on the spec level rather than the test level.

[iNikem]

So I wonder if it's better to use ConfigUtils everywhere for now, until we figure out our desired end state?

Does anybody have an idea how to solve our current problem when every test can its own, conflicting, configuration?

[iNikem]

My only idea is to have two different implementations of Config or Config storage: current one with io.opentelemetry.instrumentation.api.config.Config#INSTANCE for production, and another one with e.g. ThreadLocal storage for tests.

WDYT @mateuszrzeszutek @anuraaga @trask ?

[mateuszrzeszutek]

Does anybody have an idea how to solve our current problem when every test can its own, conflicting, configuration?

Exactly. My idea was to remove the conflicts altogether -- the easy fix for this issue was just changing HttpClientTracerTest to use the same values as NetPeerUtilsTest (and others), so the order in which those test execute wouldn't matter.

So I wonder if it's better to use ConfigUtils everywhere for now

I can remove all SPI calls and use ConfigUtils everywhere with consistent configuration settings, but if someone changes HttpClientTracerTest's configuration and some other seemingly unrelated tests start failing the same issue will resurface once again.

My only idea is to have two different implementations of Config or Config storage: current one with io.opentelemetry.instrumentation.api.config.Config#INSTANCE for production, and another one with e.g. ThreadLocal storage for tests.

Hmm, but this doesn't actually solve the static final problem: once you get something from Config and set as constant it stays immutable.

I can think of two options here:

• the BooleanConfigValue idea is a brilliant one and will certainly work in this situation: NetPeerUtils could store sth. like Supplier<Map<String, String>> instead of a plain map and the "real" Config implementation would always return the same map, the test one would ask Config every time;
• in case of instrumentation-api tests I can just put all the shared config in one class (which will use ConfigUtils) to set them and call this class in static{} blocks of test classes that use configuration. I think that all other modules can call ConfigUtils directly without any problems.

[mateuszrzeszutek]

Also, some explanation regarding resetInstrumentation(): I wanted to remove it not only because I don't like it (😄 ) but also because it should make implementing #1146 significantly easier.

[iNikem]

The more I think about it the more I wonder: maybe we indeed use statics too much?

[trask]

Also, some explanation regarding resetInstrumentation(): I wanted to remove it not only because I don't like it (😄 ) but also because it should make implementing #1146 significantly easier.

💯

on a different topic, an option (if it helps) for more complex config data types that need to parse the string:

ConfigValue<T> getConfigValue(Parser<T> parser)

this would allow one implementation that calls the parser once and returns an immutable ConfigValue<T>, and one implementation (for testing) that returns a ConfigValue<T> that internally calls the parser on each access using the latest property string (I don't think we care, but this could be optimized to cache the latest value and only re-parse if the underlying property string has changed)

[mateuszrzeszutek]

I've removed all SPI usage and made all tests use ConfigUtils for configuration changes. I'd like to try the ConfigValue idea in a separate PR - this one is just a fix after all.

[iNikem]

Does this fix the original issue? Or if somebody changes config in one of the api tests, it will fail again?

[mateuszrzeszutek]

It'll fail if somebody changes it.

[mateuszrzeszutek]

I've created a new issue for the next Config refactoring: #1345

[iNikem]

@anuraaga @trask do you approve?