-
Notifications
You must be signed in to change notification settings - Fork 637
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
Add the elections counter metric #4179
Add the elections counter metric #4179
Conversation
f85d7ae
to
2f10c55
Compare
2f10c55
to
207ce31
Compare
looks good, handling the ElectionsDone message is a nice way to do it. We can make use of the existing CounterMetric though, and have the tracker handle the message directly instead of having an extra service |
Sure. I will work on this. |
Oh yeah and updating the documentation would be fab too (in a separate PR actually since it requires a different reviewer) |
207ce31
to
ed32779
Compare
@timothycoleman I have handled the Elections.Done message in tracker itself but I am not 100% sure if this is the right to do it. |
public interface IElectionCounterTracker { | ||
void IncrementCounter(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public interface IElectionCounterTracker { | |
void IncrementCounter(); | |
} | |
public interface IElectionCounterTracker : IHandle<Messages.ElectionMessage.ElectionsDone> { | |
} |
ah ok sorry, i meant like this ^
ed32779
to
52e348c
Compare
I have added the IncrementCounter function because of Unit testing. This way the tests are easy to understand. We can test this with sut.Hanlde(null) if we want. |
cool, thanks yes we should test it by calling Handle, and remove the IncrementCounter method. That way the tests are using the tracker how it is supposed to be used you can construct an electionsdone message like this
|
0a3e700
to
870ef6d
Compare
870ef6d
to
dbfe9d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Added: Add the elections counter metric so users can set alerts if the number of elections over a certain period of time exceeds some number.
Fixes https://linear.app/eventstore/issue/DB-599/add-the-elections-counter-to-metrics