Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

writer: Clean up subscription dropped reporting #71

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

mjs
Copy link
Contributor

@mjs mjs commented Apr 24, 2018

  • Always report drop counts for each subscription, instead of only when a drop is detected.
  • Move the responsibility for reporting NATS drops to the statistician. This avoids a goroutine per subscription (which was probably overkill) and simplifies the code.
  • Removed a lot of logging that was of dubious value (the metric provides the same information and scales better)

@mjs
Copy link
Contributor Author

mjs commented Apr 24, 2018

@oplehto I've left the name of the NATS dropped metric as dropped because it already existed under that name in the writer. If you're not already consuming it I would like to change the name to nats_dropped to make its meaning clearer and make it consistent with the similar metric from the filter.

What do you think?

@mjs mjs requested a review from oplehto April 24, 2018 05:39
@oplehto
Copy link
Contributor

oplehto commented Apr 24, 2018

Changing the name is ok at this point. I'm not using it yet.

Copy link
Contributor

@oplehto oplehto left a comment

Choose a reason for hiding this comment

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

Ack. Ok to merge once the "dropped" -> "nats_dropped" metrics name change is done

- Always report drop counts for each subscription, instead of only
  when a drop is detected.
- Move the responsibility for reporting NATS drops to the
  statistician. This avoids a goroutine per subscription (which was
  probably overkill) and simplifies the code.
- Removed a lot of logging that was of dubious value (the metric
  provides the same information and scales better)
- Renamed "dropped" metric to "nats_dropped" to make the meaning
  clearer
@mjs mjs force-pushed the clean-up-writer-drop-metrics branch from a15e923 to d2cc5c0 Compare April 24, 2018 09:53
@mjs mjs merged commit 393a8c3 into jumptrading:master Apr 24, 2018
@mjs mjs deleted the clean-up-writer-drop-metrics branch April 24, 2018 09:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants