-
Notifications
You must be signed in to change notification settings - Fork 9
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 metric support to Feedback Listener #23
Conversation
…DStatsd; add related tests; update tests to use Counts map instead of Count due to name conflict
…ack-listener-metrics
…ack-listener-metrics
…ge to create topics only once; use different consumer groups per test
@mbotarro actually the env var is |
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.
Minor modifications before we can proceed.
feedback/listener.go
Outdated
@@ -80,13 +93,13 @@ func (l *Listener) configure() error { | |||
} | |||
l.Queue = q | |||
|
|||
broker, err := NewBroker(l.Logger, l.Config, q.MessagesChannel(), l.Queue.PendingMessagesWaitGroup()) | |||
broker, err := NewBroker(l.Logger, l.Config, nil, q.MessagesChannel(), l.Queue.PendingMessagesWaitGroup()) |
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.
I think we'd better send StatsReporters instead of nil in here.
feedback/listener.go
Outdated
if err != nil { | ||
return fmt.Errorf("error creating new broker: %s", err.Error()) | ||
} | ||
l.Broker = broker | ||
|
||
handler, err := NewInvalidTokenHandler(l.Logger, l.Config, l.Broker.InvalidTokenOutChan) | ||
handler, err := NewInvalidTokenHandler(l.Logger, l.Config, nil, l.Broker.InvalidTokenOutChan) |
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.
Same as above
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.
Great job! 🥇
This PR:
Adds support to
StatsReporter
in theListener
,Broker
andInvalidTokenHandler
.The
Listener
periodically flushes the pipeline channels’ size as a gauge metric. The flush interval can be set using the new env variablePUSHER_STATS_FLUSH_S
.The
InvalidTokenHandler
sends metrics related to the tokens deletion. Its unit tests have been updated using aStatsD
mock.Beyond that, the following modifications have been introduced:
Count
method has been added in theStatsd
Interface and its mock has been updated since there was aCount
field that caused a conflictStatsD
andStatsReporter
interface to send genericGauge
andCount
metrics with a game and platform as tagsRowsAffected
in the result. To solve that, the mock has been updated to put anull
character at the end of the query in a way that theNewResult
function is now able to correctly parse it.common.go
file was created to manage the reporters initialisation and use