Refactoring Config into @AutoValue POJO

[mateuszrzeszutek]

Refactored Config according to this comment: #1243 (review)

[trask]

Hey @mateuszrzeszutek!

Reviewing this gave me a further idea.

I was going to suggest not exposing the Config.getProperty method publicly (since it's not used outside of Config currently).

But it seems like there's a pretty good use case for custom instrumentation needing to call Config.getProperty.

And then I realized why shouldn't "bundled" instrumentation do the same?

(talking to myself) And anyways why should instrumentation-api know about instrumentation-specific configuration properties?

If this makes sense to you and @iNikem, I'll open an issue for it.

[iNikem]

Assorted thoughts follow.

If I create new instrumentation which needs a configuration option, should I go and change common Config class to add named setter for that? Seems like unnecessary coupling. Therefore, generic Config.getProperty makes sense and all instrumentations may/should use it and keep property names to themselves.

The only downside of the above: we still have to keep the common configuration page or to create "documentation per instrumentation". The latter may be a good idea, albeit work intensive.

We should re-use and not duplicate normalization logic from ConfigBuilder if possible.

The precedence of configuration, system properties -> env -> file -> SPI -> default, should ideally be in one place.

So, if I understand it correctly, some class from tooling module should collect all configuration from all known sources in the correct order, normalise it to a single instance of Properties or other map-like structure and create single Config instance (which lives in bootstrap module) from that. All instrumentations then can use it via single getProperty (or typed getLongProperty etc) method.

It seems I agree with @trask :)

[mateuszrzeszutek]

I guess no need to do this if we are going to remove most/all of these attributes from Config and primarily just store the properties map in Config

Do you suggest removing all those fields and just using getProperty() internally in Config getters?

[mateuszrzeszutek]

If I create new instrumentation which needs a configuration option, should I go and change common Config class to add named setter for that? Seems like unnecessary coupling. Therefore, generic Config.getProperty makes sense and all instrumentations may/should use it and keep property names to themselves.

There already are some properties like that: getKafkaClientPropagationEnabled() or getHystrixTagsEnabled(). I believe that we should remove them and make those instrumentations call getProperty() (typed getBooleanProperty()?) instead.
This way Config fields would only contain things that relate to the agent as a whole or are common to all instrumentations.
What do you think about it?

[trask]

I guess no need to do this if we are going to remove most/all of these attributes from Config and primarily just store the properties map in Config

Do you suggest removing all those fields and just using getProperty() internally in Config getters?

I suggest removing both the fields and the getters for instrumentation-specific config properties and also for config properties that are specific to javaagent-tooling (since no need to expose javaagent-tooling properties explicitly in instrumentation-api).

If I create new instrumentation which needs a configuration option, should I go and change common Config class to add named setter for that? Seems like unnecessary coupling. Therefore, generic Config.getProperty makes sense and all instrumentations may/should use it and keep property names to themselves.

There already are some properties like that: getKafkaClientPropagationEnabled() or getHystrixTagsEnabled(). I believe that we should remove them and make those instrumentations call getProperty() (typed getBooleanProperty()?) instead.
This way Config fields would only contain things that relate to the agent as a whole or are common to all instrumentations.
What do you think about it?

👍 (and same for javaagent-tooling which can call getProperty() or typed variants also)

[anuraaga]

Thanks a lot, just small comments. Great cleanup

[anuraaga]

Config becoming volatile is a surprisingly big change. It's going to make unit tests much easier where we currently use bytecode manipulation to turn Config from final to non-final so I'm generally happy about it though :)

We should confirm that we only call Config.get on cold paths, which I think should generally be the case as it's basically only to initialize instrumentation. In Java, the performance of Config.get().isFooEnabled() is actually significantly different when Java can know a boolean is a constant and C1 compiler can just remove the branches with no de-opt mechanism at all. So when .get is volatile, we'd expect a private static final boolean IS_FOO_ENABLED = Config.get().isFooEnabled() to replace such code inline on a hot path where we would like fast enabling/disabling of code paths

[mateuszrzeszutek]

That's a good point -- I'll look through usages of Config#get()

[anuraaga]

Might add a recommendation to store config values as constants if they're referenced in performance-sensitive code.

[mateuszrzeszutek]

I suggest removing both the fields and the getters for instrumentation-specific config properties and also for config properties that are specific to javaagent-tooling (since no need to expose javaagent-tooling properties explicitly in instrumentation-api).

There already are some properties like that: getKafkaClientPropagationEnabled() or getHystrixTagsEnabled(). ...

👍 (and same for javaagent-tooling which can call getProperty() or typed variants also)

Might add a recommendation to store config values as constants if they're referenced in performance-sensitive code.

Great -- I'll create a separate GH issue for that. I can implement all those fixes right away, I just want to stop this PR from growing in scope (and get it merged, of course).

[trask]

hey @mateuszrzeszutek, heads up, I merged master into your branch, but build not passing due to #1280, once that is merged you can merge master again and should be good to go 👍

[trask]

@anuraaga @iNikem heads up there's a conflict between this PR (which replaces ExporterConfig with Properties) and #1262 (which uses ExporterConfig)

[trask]

Thanks @mateuszrzeszutek!

[iNikem]

I usually prefer method names like doSomethingInternal vs internalDoSomething. What do others think?

[anuraaga]

I have slight preference for internalDoSomething - gets the point across that it's internal sooner. Another option could be to make the access package-private and move the method to an InternalConfigAccess type of class.

[trask]

interesting, I've typically done doSomethingInternal, because it is often a delegate of doSomething, but I like the reasoning of putting internal in front to "get the point across that it's internal sooner"