[File based config] Initial configuration refactoring for profiler#2505
[File based config] Initial configuration refactoring for profiler#2505robsunday merged 11 commits intosignalfx:mainfrom
Conversation
| @Override | ||
| public void customize(AutoConfigurationCustomizer autoConfiguration) { | ||
| autoConfiguration.addPropertiesSupplier(this::defaultProperties); | ||
| public static void log(ConfigProperties config) { |
There was a problem hiding this comment.
[for reviewer] This method was moved from ConfigurationLogger class to follow an approach present in SnapshotProfilingConfiguration class.
It mostly logs what utility methods of Configuration class return, not the real property values from ConfigProperties.
This makes more sense, I think, because in the logs you can see what the code is actually using. This is also consistent with the implementation of SnapshotProfilingConfiguration class.
|
|
||
| private boolean notClearForTakeoff(ConfigProperties config) { | ||
| if (!config.getBoolean(CONFIG_KEY_ENABLE_PROFILER, false)) { | ||
| if (!SplunkConfiguration.isProfilerEnabled(config)) { |
There was a problem hiding this comment.
[for reviewer] Use utility method of Configuration instead of direct accessing the property from ConfigProperties.
| logger.info("-----------------------"); | ||
| } | ||
|
|
||
| Map<String, String> defaultProperties() { |
There was a problem hiding this comment.
[for reviewer] There is no point of setting these defaults as every utility method from this class declares default value when reading property value from ConfigProperties
There was a problem hiding this comment.
The point is that you have one place to go where you can see all the defaults... but it's fine to remove.
| } | ||
|
|
||
| private Duration getCustomInterval() { | ||
| Duration customInterval = config.getDuration(CONFIG_KEY_CALL_STACK_INTERVAL, Duration.ZERO); |
There was a problem hiding this comment.
[for reviewer] Default here did not make any sense because Configuration.defaultProperties() did set another default for it: Duration.ofSeconds(10).
Default Duration.ofSeconds(10) is also returned from Configuration::getCallStackInterval
| private static final Logger logger = Logger.getLogger(Configuration.class.getName()); | ||
| private static final boolean HAS_OBJECT_ALLOCATION_SAMPLE_EVENT = getJavaVersion() >= 16; | ||
|
|
||
| private static final String DEFAULT_RECORDING_DURATION = "20s"; |
There was a problem hiding this comment.
[for reviewer] This value is changed to avoid confusion due to discrepancy between env var config and declarative config data types accepted for duration properties.
Env var config supports nonstandard format with numeric value specified together with a time unit.
This is incompatible with the declarative config where specification clearly states that duration must be provided as an integer number.
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
A few small comments, but totally fine to merge. Thanks!
| logger.info("-----------------------"); | ||
| } | ||
|
|
||
| Map<String, String> defaultProperties() { |
There was a problem hiding this comment.
The point is that you have one place to go where you can see all the defaults... but it's fine to remove.
profiler/src/main/java/com/splunk/opentelemetry/profiler/Configuration.java
Outdated
Show resolved
Hide resolved
| public static final boolean DEFAULT_MEMORY_ENABLED = false; | ||
| public static final Duration DEFAULT_CALL_STACK_INTERVAL = Duration.ofSeconds(10); | ||
| public static final boolean DEFAULT_INCLUDE_INTERNAL_STACKS = false; | ||
| public static final boolean DEFAULT_TRACING_STACKS_ONLY = false; | ||
| private static final int DEFAULT_STACK_DEPTH = 1024; | ||
| private static final boolean DEFAULT_MEMORY_EVENT_RATE_LIMIT_ENABLED = true; |
There was a problem hiding this comment.
When someone asks what the default value behavior is for foo, you're going to have to go hunting now. I think this makes it way less findable, but whatever.
There was a problem hiding this comment.
I think there is no perfect solution here. Both approaches have pros and cons. I decided to refactor it because it made the class smaller and simplified analyzing of methods' behavior. You see immediately what is the default value if you look at method's code. I left only definitions of defaults that require some additional processing / heap allocation
…guration.java Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
Minor logging improvements
3 participants
Initial PR for profiler support with declarative config.
It is a cleanup and refactoring of profiler Configuration class. This refactoring makes Configuration class reusable between env var config and declarative config approach.
There were some inconsistencies how code retrieved values from the config. In most places the code called utility methods form Configuration class, but in some places it called directly ConfigProperties::getXXX() instead (see JfrActivator, JfrSettingsOverride). Now the code consistently use Configuration class methods.