You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/10/05 11:41:01 UTC
[GitHub] [qpid-dispatch] grs opened a new pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
grs opened a new pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858
Limitations at present:
* bytes-in not recorded for http1
* neither bytes-in nor bytes-out recorded for http2
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] grs commented on a change in pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#discussion_r499816630
##########
File path: src/adaptors/http1/http1_server.c
##########
@@ -712,6 +713,8 @@ static int _server_rx_response_cb(h1_codec_request_state_t *hrs,
"[C%"PRIu64"][L%"PRIu64"] HTTP response received: status=%d phrase=%s version=%"PRIi32".%"PRIi32,
hconn->conn_id, hconn->in_link_id, status_code, reason_phrase ? reason_phrase : "<NONE>",
version_major, version_minor);
+ hreq->base.stop = qd_timer_now();
Review comment:
Rats! I missed the start on the server in the refactoring... it should be in _create_request_context when the hreq is created.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#discussion_r499780559
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -1202,8 +1220,11 @@ uint64_t qdr_http1_client_core_link_deliver(qdr_http1_adaptor_t *adaptor,
_client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
assert(rmsg && rmsg->dlv == delivery);
- if (!rmsg->dispo)
+ if (!rmsg->dispo) {
rmsg->dispo = _encode_response_message(hreq, rmsg);
+ hreq->base.stop = qd_timer_now();
Review comment:
Question: is the intention to end the timing duration at the point where the response message is completely encoded? If so this may not be the correct place to stop the timer since _encode_response_message() may return zero (message incomplete).
Other Q: is the timer expected to keep running across INFO responses?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] grs closed pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
grs closed pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] grs commented on a change in pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#discussion_r499788668
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -1202,8 +1220,11 @@ uint64_t qdr_http1_client_core_link_deliver(qdr_http1_adaptor_t *adaptor,
_client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
assert(rmsg && rmsg->dlv == delivery);
- if (!rmsg->dispo)
+ if (!rmsg->dispo) {
rmsg->dispo = _encode_response_message(hreq, rmsg);
+ hreq->base.stop = qd_timer_now();
Review comment:
Not sure what you mean by INFO responses? The point of the start and stop times is just to compute the latency for the request so we can track the maximum latency seen over time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#issuecomment-703584883
Noticed the following in Travis logs. It's not introduced in this PR, just something that was already on the branch.
```
/home/travis/build/apache/qpid-dispatch/src/adaptors/http1/http1_server.c:543:54: error: use of logical '||' with constant operand [-Werror,-Wconstant-logical-operand]
uint64_t dispo = hreq->request_dispo || PN_MODIFIED;
^ ~~~~~~~~~~~
/home/travis/build/apache/qpid-dispatch/src/adaptors/http1/http1_server.c:543:54: note: use '|' for a bitwise operation
uint64_t dispo = hreq->request_dispo || PN_MODIFIED;
^~
|
1 error generated.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
kgiusti commented on pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#issuecomment-703768765
@jiridanek
I'll fix that boolean issue in a separate patch, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] grs commented on a change in pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#discussion_r499817189
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -1202,8 +1220,11 @@ uint64_t qdr_http1_client_core_link_deliver(qdr_http1_adaptor_t *adaptor,
_client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
assert(rmsg && rmsg->dlv == delivery);
- if (!rmsg->dispo)
+ if (!rmsg->dispo) {
rmsg->dispo = _encode_response_message(hreq, rmsg);
+ hreq->base.stop = qd_timer_now();
Review comment:
Thanks! I'll look at request_complete and try and move the rcording of the request there (and will mark the end time there also).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#discussion_r499808734
##########
File path: src/adaptors/http1/http1_server.c
##########
@@ -712,6 +713,8 @@ static int _server_rx_response_cb(h1_codec_request_state_t *hrs,
"[C%"PRIu64"][L%"PRIu64"] HTTP response received: status=%d phrase=%s version=%"PRIi32".%"PRIi32,
hconn->conn_id, hconn->in_link_id, status_code, reason_phrase ? reason_phrase : "<NONE>",
version_major, version_minor);
+ hreq->base.stop = qd_timer_now();
Review comment:
Where is this timer started?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] grs commented on a change in pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#discussion_r499788210
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -1202,8 +1220,11 @@ uint64_t qdr_http1_client_core_link_deliver(qdr_http1_adaptor_t *adaptor,
_client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
assert(rmsg && rmsg->dlv == delivery);
- if (!rmsg->dispo)
+ if (!rmsg->dispo) {
rmsg->dispo = _encode_response_message(hreq, rmsg);
+ hreq->base.stop = qd_timer_now();
Review comment:
Well, I guess to some extent that is up for discussion. Probably it would be better to wait until the reponses was fully encoded though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #858: DISPATCH-1779: initial http stats for http1 and http2 adaptors
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #858:
URL: https://github.com/apache/qpid-dispatch/pull/858#discussion_r499807867
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -1202,8 +1220,11 @@ uint64_t qdr_http1_client_core_link_deliver(qdr_http1_adaptor_t *adaptor,
_client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
assert(rmsg && rmsg->dlv == delivery);
- if (!rmsg->dispo)
+ if (!rmsg->dispo) {
rmsg->dispo = _encode_response_message(hreq, rmsg);
+ hreq->base.stop = qd_timer_now();
Review comment:
By INFO I mean 100 Continue. In the case of 100 Continue this codepath will be executed twice: once when the 100 Continue is encoded and again when the terminal response arrives. So basically there are two response messages for a single request.
I'm afraid I'm being dense regarding the definition of latency - or more specifically what is included in the duration we're calling latency. I'm assuming latency is going to include the time the server spends processing the request if we're running the timer until the response is encoded (otherwise we could try stopping the timer when the server sends the terminal disposition for the request).
If we're timing the whole enchilada (server processing and full response encoding) we'd probably want to stop the timer in the request_complete callback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org