Config property util cleanup #15809
Config property util cleanup #15809
Conversation
github-actions
bot
added
the
test native
zeitlinger
force-pushed
the
config-property-util-cleanup
branch
from
dcf6adf to
a510da4
Compare
| // this test uses reflection to access fields generated by FieldBackedProvider | ||
| // internal-reflection needs to be disabled because it removes these fields from reflection results. | ||
| jvmArgs("-Dotel.instrumentation.internal-reflection.enabled=false") | ||
| jvmArgs("-Dotel.javaagent.internal-reflection.enabled=false") |
There was a problem hiding this comment.
I don't get why you'd want to introduce a special property for this instead of using the standard instrumentation disabling flag.
There was a problem hiding this comment.
This is a setting that must use properties even when declarative config is used - because it's needed at an early phase.
Since declarative config uses a different approach, it would be confusing to keep the same syntax.
There was a problem hiding this comment.
I still don't quite get why the standard way of disabling the instrumentations wouldn't work. Is it because of reading the property in AgentCachingPoolStrategy? If so I think reading the property there isn't really needed, it allows skipping a check, but that check is cheap anyway.
There was a problem hiding this comment.
yes, it's because of that check
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. | ||
| */ | ||
| public final class DeprecatedLibraryConfigPropertiesUtil { |
There was a problem hiding this comment.
I don't see what we gain from this change.
There was a problem hiding this comment.
it allows the methods to be deprecated - this has caused some confusion in a previous PR
@trask fyi
There was a problem hiding this comment.
I think things are getting clearer now and we could avoid the rename and instead deprecate it saying will be removed in 3.0
There was a problem hiding this comment.
It's not a rename - the class is split to distinguish the cases that are deprecated from the ones we still need in the future (which are few, but it's still confusing to understand right now)
…d to otel.javaagent.testing.thread-propagation-debugger.enabled
…gent.testing.internal-reflection.enabled
zeitlinger
force-pushed
the
config-property-util-cleanup
branch
from
48d8630 to
508c0dc
Compare
|
@trask please have a look |
3 participants
Fixes #15807