-
Notifications
You must be signed in to change notification settings - Fork 149
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
add configurable rate limiter for jar collector processing #183
Conversation
this.ignoreJars = new ArrayList<>(agentConfig.getIgnoreJars()); | ||
int jarsPerSecond = agentConfig.getJarCollectorConfig().getJarsPerSecond(); | ||
if (jarsPerSecond <= 0) { | ||
logger.log(Level.INFO, "Jars per second must be greater than 0. Defaulting to {0}.", DEFAULT_JARS_PER_SECOND); |
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.
Should we disable if 0 and use the default if negative?
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 could do that. I was thinking through that question myself and came to the conclusion that you could effectively disable by setting the config to really high number. Like, if someone set it to 1_000_000_000, there's no way the single thread processing these jars could run into that bound.
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'm ok with leaving this behavior as is and providing public documentation for the jar_collector
config so that customers can disable it if desired.
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.
This looks great to me. Let's ship it.
Along with this PR I think we should update the public agent config doc to include a section on the jar_collector
config options:
jar_collector:
enabled: true # default true
max_class_loaders: 300 # default 5000
skip_temp_jars: true # default true
jars_per_second: 5 # default 10
It doesn't look like we publicly document any of these config settings.
Since releasing doc updates is blocked by the release I've added a story for us to follow up with that work: #192
this.ignoreJars = new ArrayList<>(agentConfig.getIgnoreJars()); | ||
int jarsPerSecond = agentConfig.getJarCollectorConfig().getJarsPerSecond(); | ||
if (jarsPerSecond <= 0) { | ||
logger.log(Level.INFO, "Jars per second must be greater than 0. Defaulting to {0}.", DEFAULT_JARS_PER_SECOND); |
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'm ok with leaving this behavior as is and providing public documentation for the jar_collector
config so that customers can disable it if desired.
Closes #179.