Skip to content
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

[jmxfetch] run() will be non-static for App #3190

Closed
wants to merge 3 commits into from
Closed

Conversation

truthbk
Copy link
Member

@truthbk truthbk commented Nov 9, 2021

This is a very small PR to address a small change we expect to implement in the JMXFetch lib, by which the App.run() method will no longer be static. It is currently a nasty piece of code we have in JMXFetch we would like to get rid of. It should be of minor impact as it still seems very self contained within the tracer use of the lib.

@truthbk truthbk requested a review from a team as a code owner November 9, 2021 18:35
@truthbk
Copy link
Member Author

truthbk commented Nov 9, 2021

note: I can see the tests failing until we actually release the new JMXFetch version.

Copy link
Contributor

@devinsba devinsba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me though obviously we can't merge until the JMXFetch release

@@ -100,6 +100,7 @@ private static void run(final StatsDClientManager statsDClientManager, final Con
configBuilder.checkPeriod(checkPeriod);
}
final AppConfig appConfig = configBuilder.build();
private App app = new App(appConfig);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private is unnecessary here but final would be good

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I originally had that as a class attribute, then moved it into the run() method. Will mark final.

@truthbk
Copy link
Member Author

truthbk commented Nov 9, 2021

This is the PR on the jmxfetch side: DataDog/jmxfetch#376

@bm1549 bm1549 added the comp: jmx-fetch JMX fetch label Oct 24, 2023
@mcculls
Copy link
Contributor

mcculls commented Nov 16, 2023

Closing as stale, this change was applied in #3236

@mcculls mcculls closed this Nov 16, 2023
@mcculls mcculls deleted the jaime/newjmxfetch branch November 16, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants