[File based config] Cleanup of customizers that can be replaced by YAML snippets#2496
[File based config] Cleanup of customizers that can be replaced by YAML snippets#2496robsunday merged 7 commits intosignalfx:mainfrom
Conversation
There was a problem hiding this comment.
[for reviewer] This file may be added back in one of subsequent PRs if it is needed for profiler.
| import java.util.logging.Logger; | ||
|
|
||
| @AutoService(DeclarativeConfigurationCustomizerProvider.class) | ||
| public class WebengineSpanProcessorCustomizerProvider |
There was a problem hiding this comment.
I'm not sure we want to remove this. @breedx-splk wdyt?
There was a problem hiding this comment.
I also had doubts, but since it can be replaced with maximum 3 lines in YAML (see YAML snippets doc I'm working on) I decided to remove it.
But if you think it is worth keeping I'l' revert it.
There was a problem hiding this comment.
In my opinion this boils down to whether we wish to have this opt-in or opt-out. If a feature is opt-in then having user configure it in the yaml is fine. If it is opt-out then it should be enabled by default, unless user specifically disables it in yaml. Imo you should discuss it with Andrzej.
There was a problem hiding this comment.
It makes sense. @breedx-splk - what is your opinion on this?
There was a problem hiding this comment.
In my opinion this boils down to whether we wish to have this opt-in or opt-out. If a feature is opt-in then having user configure it in the yaml is fine. If it is opt-out then it should be enabled by default, unless user specifically disables it in yaml. Imo you should discuss it with Andrzej.
I agree with this completely, and I think this is what we should be doing for declarative config. We could possibly revise this approach in the future, if we are providing or generating configs for users, but even then it's a bit risky for things that we really do want to be enabled or otherwise turned on by default.
There was a problem hiding this comment.
I reverted this file and then I realized that currently there is no way to opt out from using WebengineSpanProcessor...
Maybe I should remove it again? Alternatively I could add some property under .instrumentation/development.java yaml node, but it doesn't look right to me.
What do you think?
There was a problem hiding this comment.
At the end of the day, we just need a way for users to have the webengine span processor active in our distro. If they can't turn it off today, then fine, it needs to remain active even when using declarative config. We use autoconfigure to hook into that today, and because declarative config bypasses autoconfig, we'll need a way to enable that span processor.
There was a problem hiding this comment.
OK, so it'll stay then.
| addAdditionalPropertyIfAbsent(properties, "otel.instrumentation.spring-batch.enabled", true); | ||
| addAdditionalPropertyIfAbsent( | ||
| properties, "otel.instrumentation.spring-batch.item.enabled", true); |
There was a problem hiding this comment.
Correct me if I'm wrong, but these should be treated as opt-out, and I should revive them, right?
3 participants
As we agreed - amount of custom code must be minimized.
This PR removes some customization code that is no longer needed and should be replaced by YAML templates/snippets