-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable decorators via config #1152
Conversation
@@ -596,6 +596,10 @@ public boolean isJmxFetchIntegrationEnabled( | |||
return jmxFetchIntegrationEnabled(integrationNames, defaultEnabled); | |||
} | |||
|
|||
public boolean isDecoratorEnabled(final String name) { | |||
return getBooleanSettingFromEnvironment("trace." + name.toLowerCase() + ".enabled", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewsouthard1 any objections to this config setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
new Status5XXDecorator(), | ||
new URLAsResourceName())) { | ||
|
||
if (Config.get().isDecoratorEnabled(decorator.getClass().getSimpleName())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize I'm a bit late, but none-the-less...
I think it is better to have a separate name other than the class name for any configuration setting.
Fortunately, It won't be hard for us to make that change in the future.
FYI, system properties are case sensitive, so the property needs to be set as lowercase. I'll update the PR description. |
Adds the ability to disable individual decorators with config.
ex:
-Ddd.trace.status404decorator.enabled=false
Note: system properties are case sensitive, and we do a
toLowerCase
, so the system property should also be all lowercase.Closes #1143