You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "lzx404243 (via GitHub)" <gi...@apache.org> on 2023/09/13 19:02:20 UTC
[GitHub] [trafficserver] lzx404243 opened a new pull request, #10409: Fixed HTTP method metrics not updating
lzx404243 opened a new pull request, #10409:
URL: https://github.com/apache/trafficserver/pull/10409
Currently, the HTTP request method metrics such as `proxy.process.http.get_requests` are not updating properly for non-tunneled traffic. This PR fixes this and added some test coverage.
In additional, while I am here, I cleaned up the redundant call to `build_error_response()` in `HttpTransact::SelfLoop`, as this is already called right before in `HttpTransact::will_this_request_self_loop()`, with the same arguments but more informative reason phrase.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #10409: Fixed HTTP method metrics not updating
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #10409:
URL: https://github.com/apache/trafficserver/pull/10409
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #10409: Fixed HTTP method metrics not updating
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #10409:
URL: https://github.com/apache/trafficserver/pull/10409#discussion_r1327475334
##########
proxy/http/HttpTransact.h:
##########
@@ -956,6 +956,8 @@ class HttpTransact
return std::min(cur_parent_max_attempts, txn_conf->parent_connect_attempts) - 1;
}
+ bool is_method_stats_incremented = false;
Review Comment:
Fixed.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a diff in pull request #10409: Fixed HTTP method metrics not updating
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #10409:
URL: https://github.com/apache/trafficserver/pull/10409#discussion_r1327212690
##########
proxy/http/HttpTransact.h:
##########
@@ -956,6 +956,8 @@ class HttpTransact
return std::min(cur_parent_max_attempts, txn_conf->parent_connect_attempts) - 1;
}
+ bool is_method_stats_incremented = false;
Review Comment:
This feels like an odd place to add this member. Can you move it up where all the other bool members are?
##########
proxy/http/HttpTransact.cc:
##########
@@ -891,7 +891,6 @@ HttpTransact::SelfLoop(State *s)
{
TxnDebug("http_trans", "Request will selfloop.");
bootstrap_state_variables_from_request(s, &s->hdr_info.client_request);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Direct self loop detected", "request#cycle_detected");
Review Comment:
Is this line intentionally removed ?
##########
proxy/http/HttpTransact.cc:
##########
@@ -5807,6 +5783,34 @@ HttpTransact::initialize_state_variables_from_request(State *s, HTTPHdr *obsolet
s->dns_info.lookup_name = s->server_info.name;
}
+void
Review Comment:
While we're at it, would this not be better / nicer with a switch statement ?
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #10409: Fixed HTTP method metrics not updating
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #10409:
URL: https://github.com/apache/trafficserver/pull/10409#issuecomment-1721184552
Can you update this with the new metrics? :)
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #10409: Fixed HTTP method metrics not updating
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #10409:
URL: https://github.com/apache/trafficserver/pull/10409#discussion_r1327481924
##########
proxy/http/HttpTransact.cc:
##########
@@ -891,7 +891,6 @@ HttpTransact::SelfLoop(State *s)
{
TxnDebug("http_trans", "Request will selfloop.");
bootstrap_state_variables_from_request(s, &s->hdr_info.client_request);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Direct self loop detected", "request#cycle_detected");
Review Comment:
Yes this is not related to the fix. This is a cleanup while I am here and I noted that in the description.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #10409: Fixed HTTP method metrics not updating
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #10409:
URL: https://github.com/apache/trafficserver/pull/10409#discussion_r1327478301
##########
proxy/http/HttpTransact.cc:
##########
@@ -5807,6 +5783,34 @@ HttpTransact::initialize_state_variables_from_request(State *s, HTTPHdr *obsolet
s->dns_info.lookup_name = s->server_info.name;
}
+void
Review Comment:
I tried, but values like `HTTP_WKSIDX_GET` is not compile-time constants so I left it the way it was.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on pull request #10409: Fixed HTTP method metrics not updating
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on PR #10409:
URL: https://github.com/apache/trafficserver/pull/10409#issuecomment-1721492578
> Can you update this with the new metrics? :)
@zwoop Updated to use the new metrics. I like it 😄 .
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org