Skip to content
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

Enable prometheus logging for buildfarm:worker read on client cancel #1417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amishra-u
Copy link
Contributor

Problem

Currently we don't see any metrics with status cancelled for buildfarm:worker read. This issue arises because the onCancelHandler does not invoke the responseObserver.onError method. As a result, the reportEndMetrics function, responsible for logging prometheus metrics, is not triggered.

Solution

Added code to ensure that the responseObserver.onError method is called when the client initiates a cancellation request. Additionally, added a check in the ServerStreamCallObserver implementation to avoid the call already closed error, which could occur if the buildfarm:worker has already called onComplete or onError (realistically onComplete) by the time the cancellation request from the client (buildfarm:server) arrives.

@amishra-u amishra-u marked this pull request as ready for review August 6, 2023 03:28
@amishra-u amishra-u requested a review from werkt as a code owner August 6, 2023 03:28
@amishra-u
Copy link
Contributor Author

@werkt Can you please take a look. This is minor update with no functionality change.

Copy link
Collaborator

@werkt werkt left a comment

Choose a reason for hiding this comment

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

This is not a change I'm interested in making. You're accommodating an interceptor for its behavior by changing the usage, inducing a registration with a loop back through an already cancelled handle, and the affected domain for this is the single getBlob, ignoring all the other calls where this takes place.

This is a problem with the prometheus interceptor, I recommend you make the change there - it should handle cancels by presenting metrics.

@@ -595,8 +595,7 @@ public void getBlob(
ServerCallStreamObserver<ByteString> blobObserver,
RequestMetadata requestMetadata) {
throwIfStopped();
bsStub
.get()
deadlined(bsStub)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change isn't in line with the description of the commit.

@amishra-u
Copy link
Contributor Author

This is not a change I'm interested in making. You're accommodating an interceptor for its behavior by changing the usage, inducing a registration with a loop back through an already cancelled handle, and the affected domain for this is the single getBlob, ignoring all the other calls where this takes place.

This is a problem with the prometheus interceptor, I recommend you make the change there - it should handle cancels by presenting metrics.

Thanks for feedback, will make the required changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants