Avoid usage of static getters when accessing configuration parameters#863
Avoid usage of static getters when accessing configuration parameters#863
Conversation
labbati
changed the title
…i/non-static-config
tylerbenson
left a comment
tylerbenson
left a comment
There was a problem hiding this comment.
Bunch of comments. If you'd rather leave as is, LMK and I'll approve.
| when: | ||
| BaseDecorator dec = withSystemProperty("dd.${integName}.analytics.enabled", "true") { | ||
| withSystemProperty("dd.${integName}.analytics.sample-rate", "$sampleRate") { | ||
| ConfigUtils.resetConfig() |
There was a problem hiding this comment.
Want to take this opportunity to remove withSystemProperty and migrate to withConfigOverride?
| // loads opentracing before bootstrap classpath is setup | ||
| // so we declare tracer as an object and cast when needed. | ||
| protected static final Object TEST_TRACER; | ||
| protected static Object TEST_TRACER; |
There was a problem hiding this comment.
I'd prefer if we could avoid this somehow.
There was a problem hiding this comment.
Unfortunately we are accessing the global config from within the tracer constructor so I fear that this is necessary until we proceed with our future plan: i.e.
- injecting the config into the tracer
- always use that one from within the tracer
- when you need to access it from a static method then use the tracer to retrieve it
So basically the tracer will become our source of truth for the config object, so please see this as temporarily until we have time to implement the second part of the plan :)
Moreover, I would notice that here we are in a test, not in code, so having a non final variable it is acceptable IMHO.
There was a problem hiding this comment.
I realize we are in a test, but I think it leaves opportunity for confusing bugs down the road. If we can't work around it, it's fine.
There was a problem hiding this comment.
I just realized this should not be necessary. Looking into it.
dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/ConfigUtils.groovy
Outdated
Show resolved
Hide resolved
| * tot he new config instance. | ||
| */ | ||
| static void resetConfig() { | ||
| static void resetConfig(preserveRuntimeId = false) { |
There was a problem hiding this comment.
Should we make preserveRuntimeId always true? Not sure we really need to test with the false case.
There was a problem hiding this comment.
I think this makes sense. Let me see what the consequences are.
| static { | ||
| ConfigUtils.makeConfigInstanceModifiable() | ||
| System.setProperty("dd.trace.methods", "package.ClassName[method1,method2];${ConfigTracedCallable.name}[call]") | ||
| ConfigUtils.resetConfig() |
There was a problem hiding this comment.
Suggestion: add a check inside makeConfigInstanceModifiable to ensure it is only run once (it won't hurt anything to run again... it's just unnecessary).
Then change this sequence to something more like this:
ConfigUtils.updateConfig {
System.setProperty()
}This would implicitly apply the makeConfigInstanceModifiable if necessary and resetConfig.
There was a problem hiding this comment.
I like the idea. 👍 much better
|
|
||
| traceEnabled = getPropertyBooleanValue(properties, TRACE_ENABLED, parent.traceEnabled); | ||
| integrationsEnabled = | ||
| getPropertyBooleanValue(properties, INTEGRATIONS_ENABLED, parent.integrationsEnabled); |
There was a problem hiding this comment.
It would be nice if we could restructure this class so we didn't have to duplicate all this between each constructor. Perhaps we should move to a builder instead?
There was a problem hiding this comment.
I've tried that in the past but could not find a good way to do that, unfortunately. I do not think builder would help here: you would still have to handle two different config sources separately listing all fields (or builder methods) - it probably would work, but it would still be very verbose...
There was a problem hiding this comment.
I would call it out of scope from this PR. And yes I totally agree with your concern. Let's wrap up our minds if we find a better approach. I would not block this PR tough.
| } | ||
| } | ||
|
|
||
| static void makeConfigInstanceModifiable() { |
There was a problem hiding this comment.
Do you want a check to prevent this from being run multiple times?
| */ | ||
| static void resetConfig() { | ||
| // Ensure the class was retransformed properly in AgentTestRunner.makeConfigInstanceModifiable() | ||
| static void resetConfig(preserveRuntimeId = true) { |
There was a problem hiding this comment.
Seems this argument is no longer needed.
| // loads opentracing before bootstrap classpath is setup | ||
| // so we declare tracer as an object and cast when needed. | ||
| protected static final Object TEST_TRACER; | ||
| protected static Object TEST_TRACER; |
There was a problem hiding this comment.
I realize we are in a test, but I think it leaves opportunity for confusing bugs down the road. If we can't work around it, it's fine.
|
@tylerbenson ack to you just in case you like to take a final look after your latest comments. |
3 participants
There were many places in our code where we directly accessed configuration properties directly using static getter instead of reading those values from the singleton configuration instance.
While this can be seen as the first step into a large project to improve our configuration mechanism, for now we just want to avoid usage of those static method from outside the Config class.