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.

Sign up for GitHub

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

Jump to bottom

Adds a configurable period for the 1st bean refresh #349

Merged
merged 9 commits into from
Jan 23, 2021
Merged

Adds a configurable period for the 1st bean refresh #349

merged 9 commits into from
Jan 23, 2021

Conversation

ian28223
Copy link
Contributor

What this PR does

Adds a configurable period for the 1st bean refresh.

Additional info

  • Configurable via refresh_beans_initial - time in seconds for jmxfetch to wait to refresh the beans for the first time.
  • Succeeding bean refresh still adheres to refresh_beans parameter.
  • Setting a value greater than the refresh_beans will default to a value equal to refresh_beans.

Motivation

  • AC-737
  • Useful for Java applications that are lazy loaded and may take some time after application startup before actually being exposed.

@ian28223
Add refresh_beans_initial
a20be8e 
Configurable parameter that determines the amount in seconds before performing the 1st bean refresh. Succeeding bean refresh controlled by "refresh_beans"
Decided on a default of 2m30s
@ian28223
Fix comments
03bb2a5
@ian28223
convert to reusable function
9ab7ff3
truthbk
truthbk reviewed Jan 22, 2021
View reviewed changes
Copy link
Member

@truthbk truthbk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a couple of comments, also, I'm not sure if we currently have a unit test to cover the refresh beans logic, but it would be nice to have otherwise.

Note that tests appear to be failing.

src/main/java/org/datadog/jmxfetch/Instance.java Outdated Show resolved Hide resolved
src/main/java/org/datadog/jmxfetch/Instance.java Show resolved Hide resolved
@ian28223 ian28223 added the wip label Jan 22, 2021
@ian28223
Copy link
Contributor Author

Just a couple of comments, also, I'm not sure if we currently have a unit test to cover the refresh beans logic, but it would be nice to have otherwise.

I'd work on the unit test either here (if it doesn't make it before freeze).. or in another PR

@truthbk truthbk removed the wip label Jan 23, 2021
@truthbk truthbk merged commit 0f9f88a into master Jan 23, 2021
@truthbk truthbk deleted the ian.bucad/jmxfetch_init_refresh_beans branch January 23, 2021 19:10
@truthbk truthbk added this to the 0.42.0 milestone Jan 24, 2021
@ian28223 ian28223 mentioned this pull request Jan 29, 2021
Merged
@tylerbenson
Copy link
Contributor

I think this resolves #292.

This was referenced Apr 26, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@truthbk truthbk truthbk approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
0.42.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ian28223 @tylerbenson @truthbk