Bean subscription #426
Bean subscription #426
Conversation
scottopell
force-pushed
the
feat/bean-listener
branch
from
c6c4270
to
75d9296
Compare
|
| @Override | ||
| public void run() { | ||
| while (true) { | ||
| MBeanServerNotification mbs; |
There was a problem hiding this comment.
NIT: Do you need to declare this outside the try?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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
scottopell
force-pushed
the
feat/bean-listener
branch
from
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
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.NotificationListeneris 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