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

restart the trace reporters ticker when the traceTickInterval changes #504

Merged
merged 1 commit into from
Feb 1, 2018

Conversation

briantopping
Copy link
Contributor

While adding a warning about nothing to collect, I think I noticed a bug with the comparison about traceTickInterval.

The warning is good when people don't even know if Kamon is loading. They'll at least see something in their logs this way.

@briantopping
Copy link
Contributor Author

Caution: I would close this PR for potential lack of understanding, but wondering if the change on line 203 is correct.

@briantopping briantopping changed the title Add warning when nothing is expected to happen Add warning when no reporters are loaded Jan 24, 2018
@ivantopo
Copy link
Contributor

Hey @briantopping, thanks for the PR! the change in line 203 definitely has to come in, but the warning is not necessary. Reconfiguring Kamon doesn't necessarily mean that reporters should be present or not and there is already a warning log when loading reporters from configuration and nothing is in the kamon.reporters config key.

Could you please re-purpose the PR (commit message, PR title and so on) towards only fixing the issue on line 203? Thanks!

@ivantopo
Copy link
Contributor

ivantopo commented Feb 1, 2018

Thanks @briantopping 👍

I'm sorry I didn't see that you actually updated the PR before, shipped v1.0.1 a few hours ago.. this will make it to 1.0.2

@ivantopo ivantopo changed the title Add warning when no reporters are loaded restart the trace reporters ticker when the traceTickInterval changes Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants