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

Feature/observers #50

Merged
merged 18 commits into from
Apr 9, 2019
Merged

Feature/observers #50

merged 18 commits into from
Apr 9, 2019

Conversation

fralalonde
Copy link
Owner

No description provided.

mixalturek and others added 7 commits March 7, 2019 15:23
- Use `MetricValue` instead of `isize`.
- Cancel handle in example.
- Configurable thread name.
- If multiple callbacks are registered under the same conflicting key, only the last one will survive.
- It would be especially bad if the callback was re-registered periodically. The memory would increase and even all historical callbacks would be executed during each iteration.
@fralalonde
Copy link
Owner Author

  • Still missing self.notify_flush_listeners() for most output's flush()
  • on_flush requires metrics to be mut
  • the behavior for adding multiple flush listeners on sibling outputs needs to be sorted out

@fralalonde
Copy link
Owner Author

I'm not sure about the Observer struct being worth much... It could be easily replaced by a Gauge-updating closure wherever it is used, either on_flush or every scheduled.

@fralalonde fralalonde force-pushed the feature/observers branch 2 times, most recently from 188141b to 0b376e4 Compare April 8, 2019 03:38
@fralalonde fralalonde force-pushed the feature/observers branch 4 times, most recently from 18285f6 to f59d403 Compare April 9, 2019 01:36
@fralalonde
Copy link
Owner Author

fralalonde commented Apr 9, 2019

@mixalturek last call for API changes, this is ready to merge!

I'm satisfied with the new API and I believe I went back to something close to your original proposal:

    let uptime = metrics.gauge("uptime");
    metrics.observe(uptime, || 6).on_flush();

    metrics
        .observe(metrics.gauge("threads"), thread_count)
        .every(Duration::from_secs(1));

What do you think?

I'm sorry for taking so long and clobbering your original PR... Some lower-level changes were required for this to work, mainly the flush notifications and a new shared-thread scheduler (so we don't create a new thread for every observed gauge).

Your original commits are still in the commit log for posterity, if it's something you care about :)

@fralalonde fralalonde merged commit ff5c291 into master Apr 9, 2019
@fralalonde fralalonde deleted the feature/observers branch April 9, 2019 20:10
@mixalturek
Copy link
Contributor

Hi, I'm sorry, I had and have too many other tasks from many directions and didn't have time for this at all. I also received no email notification about your work until yesterday, so I fully missed everything. The API described in the comment above looks great!

This was referenced Apr 10, 2019
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