Ext-proc: Change the default logging status onGrpcClose()#41383
Ext-proc: Change the default logging status onGrpcClose()#41383melginaldi wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
…Close() Signed-off-by: Melissa Ginaldi <mginaldi@google.com>
Signed-off-by: Melissa Ginaldi <mginaldi@google.com>
Signed-off-by: Melissa Ginaldi <mginaldi@google.com>
melginaldi
requested review from
mattklein123,
tyxia and
yanavlasov
as code owners
| } | ||
|
|
||
| void Filter::onGrpcClose() { onGrpcCloseWithStatus(Grpc::Status::Aborted); } | ||
| void Filter::onGrpcClose() { onGrpcCloseWithStatus(Grpc::Status::Ok); } |
There was a problem hiding this comment.
What you are doing here is confusing, and the parameter is just used by clearAsyncState, which was Abort before https://github.com/envoyproxy/envoy/pull/40808/files, @yanjunxiang-google, is clearing Abort the correct behavior for onGrpcClose() before?
There was a problem hiding this comment.
You are correct that onGrpcClose() use to return status ABORT just like all of the fail-open cases. However, I think this is the wrong status to propagate. Tracing back onGrpcClose() is called here: https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/common/ext_proc/grpc_client_impl.h#L201 where the status is OK. I did not find another reference where the status was not OK as onGrpcError is called in such a case.
I should have included this in my previous PR (#40808) when I changed the fail-open statuses but I missed it.
There was a problem hiding this comment.
yes, I meant the
void ProcessorState::clearAsyncState() {
onFinishProcessorCall(Grpc::Status::Aborted);
It will be always clear and onFinishProcessorCall(Grpc::Status::Aborted); even for a onGrpcClose() call, so my q now, is the behavior the correct one before your previous PR? if yes, changing to Ok will change the behavior of the ext_proc.
There was a problem hiding this comment.
IIUC, before the previous PR, Envoy set the call status into ABORTED in some scenarios regardless the actual gRPC status. This caused some logging issue. These series PRs are trying to log the right gRPC call status.
There was a problem hiding this comment.
+1 to Yanjun, These PRs are trying to fix the old functionality which would only ever report ABORTED.
From my understanding, any server half close that has an error will call onGrpcError() so onGrpcClose() will only happen when the server-half closed with an OK status. However, if we feel the correct status in aborted because it was a server half close I can close this PR and delete. I also sent out #41691 which will add a boolean to extProcLogging which will track when sever half closes take place. I still feel logging OK is the current action since that was the actual grpc status but I do not want to break any backward compatability
| } | ||
|
|
||
| void Filter::onGrpcClose() { onGrpcCloseWithStatus(Grpc::Status::Aborted); } | ||
| void Filter::onGrpcClose() { onGrpcCloseWithStatus(Grpc::Status::Ok); } |
There was a problem hiding this comment.
Thanks for working on this!
I am wondering how difficult it is to add the gRPC status parameter in onGrpcClose() , like change it into onGrpcClose(Grpc::Status::GrpcStatus status). That way it's much safer to do this: Filter::onGrpcClose(Grpc::Status::GrpcStatus status) { onGrpcCloseWithStatus(status); }
There was a problem hiding this comment.
I could, but in order to change this method onGrpcClose() I would need to change the original definition since it is overriding the method: https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/common/ext_proc/grpc_client.h#L22
This would require also changing any files that override the method. I am happy to do this if we find this is the best way forward. In my previous PR I created onGrpcCloseWithStatus() to avoid this change though.
There was a problem hiding this comment.
IMHO, adding the gRPC status parameter like Filter::onGrpcClose(Grpc::Status::GrpcStatus status) is the right way to go. It's error-proof. You can also remove onGrpcCloseWithStatus().
|
will defer to @yanjunxiang-google for final check. |
Yeah, let's wait for @melginaldi to address the open comments. |
|
/wait-any |
Apologies. I forgot about this PR and am now just getting back to it |
|
/wait |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
4 participants
Commit Message: Ext-proc: Change the default logging status from ABORTED to OK for onGrpcClose()
Additional Description: onGrpcClose() is called when the external processing server half-closes with an OK status. Even though the stream is being closed the correct status to report is OK not ABORTED. If there was an error the Aborted error can be propagated with onGrpcError().
Risk Level: Low
Docs Changes: N/A
Platform Specific Features: N/A
Release Notes: If onGrpcClose() with status=OK, save the grpc status code OK instead of Aborted.
/assign @yanjunxiang-google