-
Notifications
You must be signed in to change notification settings - Fork 69
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
Bean subscription #426
base: master
Are you sure you want to change the base?
Bean subscription #426
Conversation
c6c4270
to
75d9296
Compare
|
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.
From a high level (I don't have deep knowledge of JMX) this LGTM.
Just some nits and comments you can probably ignore 😅
@Override | ||
public void run() { | ||
while (true) { | ||
MBeanServerNotification mbs; |
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.
NIT: Do you need to declare this outside the try
?
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.
No I don't, moved the decl inside the try
// matches | ||
// If so, we store the attribute so metrics will be collected from it. Otherwise we | ||
// discard it. | ||
for (Configuration conf : configurationList) { |
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.
NIT: can we split this into one or more helper functions? The nesting of multiple conditions and loops is somewhat hard to follow (mostly due to the newline expansion)
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 just realized you didn't add this code and simply moved it. Feel free to ignore this comment.
this.metricCountForMatchingAttributes += jmxAttribute.getMetricsCount(); | ||
log.debug("Added attribute {} from bean {}.", jmxAttribute, beanName); | ||
this.matchingAttributes.add(jmxAttribute); | ||
if (action.equals(AppConfig.ACTION_LIST_EVERYTHING) |
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.
NIT:
Can we unpack this if statement? I'm having a hard time quickly parsing out the truth conditions.
Maybe something like:
var shouldAddMatch = (action.equals(AppConfig.ACTION_LIST_EVERYTHING) || action.equals(AppConfig.ACTION_LIST_MATCHING) || action.equals(AppConfig.ACTION_LIST_COLLECTED)) && !this.limitReached
var shouldAddLimitedMatch = action.equals(AppConfig.ACTION_LIST_LIMITED) && this.limitReached
if (shouldAddMatch || shouldAddLimitedMatch) ...
Alternately, parenthesis may help.
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 just realized you didn't add this code and simply moved it. Feel free to ignore this comment.
return; | ||
} | ||
MBeanServerNotification mbs = (MBeanServerNotification) notification; | ||
queue.offer(mbs); |
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.
Do we want to do anything here if we get false
back? Maybe log/warn we couldn't add the MBeanServerNotification
?
# Conflicts: # tools/misbehaving-jmx-server/README.md
b93caeb
to
79ce047
Compare
…ReconnectContainer
# Conflicts: # src/main/java/org/datadog/jmxfetch/Instance.java # src/main/java/org/datadog/jmxfetch/JmxAttribute.java # src/test/java/org/datadog/jmxfetch/TestCommon.java
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.
editorial feedback
Co-authored-by: Kaylyn <kaylyn.sigler@datadoghq.com>
# Conflicts: # src/main/java/org/datadog/jmxfetch/App.java
…tion-level config
# Conflicts: # src/test/java/org/datadog/jmxfetch/TestCommon.java
eCurrently there is a periodic refresh of the beans (and their attributes) which can result in CPU spikes as these remote RMI calls are expensive.
This PR implements a "subscription mode" for instances where a
javax.management.NotificationListener
is used to subscribe to bean registration/unregistration events.This approach should spread out the expensive RMI calls over time and will reduce the total amount of calls needed.
Status @ Feb 8 2024:
This branch is showing flakey tests, so they need to be investigated before this can be merged.
Once this branch is clean, I intend to merge this and publish a SNAPSHOT build which can be used in nightly agent builds to get validation. This agent branch is required before the snapshot/nightly.
Latest flakey report:
Ran 2000 times, 4 failures found