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(pubsub): report publisher outstanding metrics #6187
feat(pubsub): report publisher outstanding metrics #6187
Conversation
Marking as draft for a bit. Based on internal testing, there's one more path where we need to upsert the topic tag on the context. |
Updated with the tag fix. With the most recent commit, the behavior we see in our testing is:
|
pubsub/flow_controller.go
Outdated
func (f *flowController) recordOutstandingBytes(ctx context.Context, n int64) { | ||
if f.purpose == flowControllerPurposeTopic { | ||
recordStat(ctx, PublisherOutstandingBytes, n) | ||
} else { |
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.
If we're not going to check the unknown value here, we should either remove that value above or explicitly check f.purpose == flowControllerPurposeSubscription
here.
pubsub/flow_controller.go
Outdated
case flowControllerPurposeTopic: | ||
recordStat(ctx, PublisherOutstandingMessages, n) | ||
case flowControllerPurposeSubscription: | ||
fallthrough |
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.
Made this fall through to maintain backwards compatibility with the old behavior. However, as written there shouldn't be any cases where a "real" client has an unknown purpose, so I'm happy to make default
a noop. Or I can remove the unknown value entirely and default to flowControllerPurposeSubscription
.
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 should remove the unknown value since flowcontroller is only used for publisher/subscribers here. Thanks!
Fixes #6180.