-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: NTRN-204 enhance subscriber monitoring #48
Conversation
} | ||
|
||
func DecQueriesToProcess() { | ||
queriesToProcess.With(prometheus.Labels{}).Inc() |
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.
.Inc()
?
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.
🤡
internal/relay/relayer.go
Outdated
@@ -73,6 +73,7 @@ func (r *Relayer) Run(ctx context.Context, tasks <-chan neutrontypes.RegisteredQ | |||
) | |||
select { | |||
case query := <-tasks: | |||
neutronmetrics.SetSubscriberTaskQueueNumElements(len(tasks)) |
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 don't think it belongs in the relayer, better move it to subscriber
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.
ah, I see the same track in the Subscriber. what's the point of two of them? isn't one enough measuring the len just before sending to the queue? also, as far as I can see, both tracks will have the same result since they will occur in the same time — on read from channel
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 have no idea how Golang scheduling works under the hood, hence I have placed calls to metrics in both places. What do you think is wise to do in this situation?
} | ||
|
||
func IncQueriesToProcess() { | ||
queriesToProcess.With(prometheus.Labels{}).Inc() |
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.
why not a gauge with .Set(subscriber.activeQueries)
? will simplify the if !queryExists
part in the processUpdateEvent
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.
In tracking task it is required to use a counter. If more people from team think it is better to use gauge here — I will switch.
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.
agree with @sotnikov-s
since @foxpy is unavailable now, i can fix it
internal/subscriber/utils.go
Outdated
out[restQuery.ID] = neutronQuery | ||
instrumenters.IncQueriesToProcess() |
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.
don't get why doubled inc?
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.
that's definitely a mistake!
# Conflicts: # internal/relay/relayer.go # internal/subscriber/subscriber.go # internal/subscriber/utils.go # internal/txsubmitchecker/txsubmitchecker.go
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.
lgtm. tested: metrics are counted up and down properly
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.
LGTM, and integration tests for kv and tx passed
Tracking task: NTRN-204.