You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/10/08 19:05:37 UTC
[GitHub] [trafficserver] traeak opened a new pull request #7259: detect and fix stms log field having epoch time ms for intercept plugins.
traeak opened a new pull request #7259:
URL: https://github.com/apache/trafficserver/pull/7259
For intercept plugins like stats_over_http and slice the stms logging field may end up being set to epoch time in ms.
This fix detects that condition inside HttpSM by only allowing TS_MILESTONE_SERVER_CLOSE to be set if
TS_MILESTONE_SERVER_CONNECT was already set.
This is a mitigation in the code.
resolves #7196
----------------------------------------------------------------
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
[GitHub] [trafficserver] bneradt commented on a change in pull request #7259: detect and fix stms log field having epoch time ms for intercept plugins.
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7259:
URL: https://github.com/apache/trafficserver/pull/7259#discussion_r518418730
##########
File path: proxy/http/HttpSM.cc
##########
@@ -2976,7 +2976,10 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p)
{
STATE_ENTER(&HttpSM::tunnel_handler_server, event);
- milestones[TS_MILESTONE_SERVER_CLOSE] = Thread::get_hrtime();
+ // The following fixes up an intercept handler which doesn't set TS_MILESTONE_SERVER_CONNECT by default.
Review comment:
To clarify that this comment applies to the check itself rather than the whole conditional block, maybe reword as:
```
// An intercept handler may not set TS_MILESTONE_SERVER_CONNECT by default. Therefore we only
// set TS_MILESTONE_SERVER_CLOSE if TS_MILESTONE_SERVER_CONNECT is set (non-zero), lest
// certain time statistics are calculated from epoch time.
```
Just a suggestion.
----------------------------------------------------------------
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
[GitHub] [trafficserver] zwoop commented on pull request #7259: detect and fix stms log field having epoch time ms for intercept plugins.
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7259:
URL: https://github.com/apache/trafficserver/pull/7259#issuecomment-768531182
Cherry-picked to v9.0.x branch.
----------------------------------------------------------------
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
[GitHub] [trafficserver] zwoop commented on pull request #7259: detect and fix stms log field having epoch time ms for intercept plugins.
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7259:
URL: https://github.com/apache/trafficserver/pull/7259#issuecomment-768531346
Cherry-picked to 8.1.x
----------------------------------------------------------------
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
[GitHub] [trafficserver] traeak merged pull request #7259: detect and fix stms log field having epoch time ms for intercept plugins.
Posted by GitBox <gi...@apache.org>.
traeak merged pull request #7259:
URL: https://github.com/apache/trafficserver/pull/7259
----------------------------------------------------------------
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
[GitHub] [trafficserver] elsloo commented on pull request #7259: detect and fix stms log field having epoch time ms for intercept plugins.
Posted by GitBox <gi...@apache.org>.
elsloo commented on pull request #7259:
URL: https://github.com/apache/trafficserver/pull/7259#issuecomment-722515865
Hey @traeak, we observed this occurring in our environment due to requests handled by an intercept plugin. I reviewed the fix provided here in the context of running an intercept plugin, and I believe that it will resolve the issue. It appears that there's only one place where the `TS_MILESTONE_SERVER_CONNECT` milestone timing is set, inside `HttpSM::do_server_open(bool)`, and if that method is not called, the subsequent logging calculations that leverage this milestone produce unexpected results as you mentioned above.
I believe we should merge this PR. 👍
----------------------------------------------------------------
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
[GitHub] [trafficserver] traeak commented on a change in pull request #7259: detect and fix stms log field having epoch time ms for intercept plugins.
Posted by GitBox <gi...@apache.org>.
traeak commented on a change in pull request #7259:
URL: https://github.com/apache/trafficserver/pull/7259#discussion_r518758608
##########
File path: proxy/http/HttpSM.cc
##########
@@ -2976,7 +2976,10 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p)
{
STATE_ENTER(&HttpSM::tunnel_handler_server, event);
- milestones[TS_MILESTONE_SERVER_CLOSE] = Thread::get_hrtime();
+ // The following fixes up an intercept handler which doesn't set TS_MILESTONE_SERVER_CONNECT by default.
Review comment:
updated PR with above comment.
----------------------------------------------------------------
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