Add support for metrics in stream response handler #738
Conversation
| /* Close the Connection if the Java Callback throws an Exception */ | ||
| aws_http_connection_close(aws_http_stream_get_connection(stream)); | ||
| } | ||
|
|
There was a problem hiding this comment.
We need to manually delete the local reference created when it's called from a C thread, (*env)->DeleteLocalRef(env, jni_metrics);, otherwise, it will result in a leak. We had the similar leak recently, referring to #734
src/native/http_request_response.c
Outdated
| if (aws_jni_check_and_clear_exception(env)) { | ||
| /* Close the Connection if the Java Callback throws an Exception */ | ||
| aws_http_connection_close(aws_http_stream_get_connection(stream)); | ||
| } |
There was a problem hiding this comment.
Debatable: I know we have this pattern of error handling from on_complete callback, but I don't really like it. Especially as we introduced http/2, seems like a overkill to kill the whole connection, but, TBH, I don't really have better idea about how to handle this.
I would suggest to at least add a warning-level log here.
There was a problem hiding this comment.
Looking at other callbacks, it seems the common practice is to log at error level (vs warning) and to raise an error as well so I've done that here. Let me know if you'd strongly prefer this be logged at warning level or that the error not be raised.
… log and raise an error when an exception occurs in the onMetrics callback
| /** | ||
| * Called right before stream is complete, whether successful or unsuccessful. | ||
| * @param stream The HTTP stream to which the metrics apply | ||
| * @param metrics The [HttpStreamMetrics] containing metrics for the given stream | ||
| */ | ||
| default void onMetrics(HttpStreamBase stream, HttpStreamMetrics metrics) { | ||
| /* Optional callback, nothing to do by default */ | ||
| } |
There was a problem hiding this comment.
nitpick: can we move the metrics callback before the complete callback? So, that it's the same order as it gets invoked
…rder of onMetrics methods in Java code to match invocation order; clean up clang-format errors
Issue #, if available: smithy-lang/smithy-kotlin#893
Description of changes: This change adds support for getting metrics as part of the Java-level
HttpStreamResponseHandler.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.