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 18:53:54 UTC

[GitHub] [trafficserver] shinrich opened a new pull request #7258: Fix proxy.process.http.current_client_transactions

shinrich opened a new pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258


   This closes #7163


----------------------------------------------------------------
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] maskit commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

maskit commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-708117045


   For the HTTP/3 question, I think everything related to the metrics should be done in `HQTransaction` and/or `HQSession`. We currently support HTTP/3 and HTTP/0.9 on QUIC, and you can think like the HTTP/0.9 support is a variation of HTTP/3. One transaction runs on one stream. Basically the same approach should work for both HTTP/2 and HTTP/3 (and HTTP/0.9 on QUIC).
   
   If you are still no sure, you can just file an issue for it. The metrics are not implemented for H3 anyway. I'm ok as long as it doesn't crash. I think H3 issues should not block 9.0.0 release.
   
   > Ultimately ProxyTransaction::release() should probably be renamed to something else. Calling super_type::release() seems somewhat obscure.
   
   I'm not sure what you are trying to solve with the renaming. If it's just for clear naming I'm fine with it, but if you are not a big fun of calling super type's function we may be able to do something like this:
   ```cpp
   virtual void ProxyTransaction::_release() = 0;
   void
   ProxyTransaction::release() {
     this->decrement_client_transactions_stat();
     this->_release()
   }
   
   Http1Transaction::_release() {
   
   }
   ```
   I understand the code above is impossible because `super_type::release()` is called from different places. However, I wonder why we can't call the decrement function from the same place on all HTTP versions. If we have a function that will be called at the end of a transaction regardless of how it ends, like TXN_CLOSE hook, we should be able to call the decrement function from there.


----------------------------------------------------------------
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] shinrich commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-708440791


   Pushed another commit.  Moved the decrement to a transaction_done superclass function.  Got rid of the release superclass function.   Now the increment is performed in new_transaction and the decrement is performed in transaction_done which seems symmetric. 
   


----------------------------------------------------------------
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 #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

zwoop commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-709695740


   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] shinrich commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-706221441


   Yes, I agree that the fact that the Http/1.1 case is not calling release() is unsettling.  However, given the nearness of the 9.0.x release, this seemed like a safer (less broadly impacting) change.  Let me take another look at see if I can work the release back in without a broader impact.


----------------------------------------------------------------
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] shinrich commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-707883767

Pushed a new more through version of a fix. Looking into this more deeply the Http2 version of this metric was also incorrect, over decrementing instead of under decrementing.

From one of our prod boxes running ATS9 without this fix
```
-bash-4.2$ traffic_ctl metric match proxy.process.http.current_client_
NOTE: using the environment variable TS_RUNROOT
proxy.process.http.current_client_connections 16299
proxy.process.http.current_client_transactions 1598007
-bash-4.2$ traffic_ctl metric match proxy.process.http2.current_client_
NOTE: using the environment variable TS_RUNROOT
proxy.process.http2.current_client_connections 52634
proxy.process.http2.current_client_streams 0
```

This PR changes ProxyTransaction::release() so all it really does is decrement the current transaction metric. It removes calling release on the associated parent session. The one place in HttpSM that directly calls ua_txn->release() is augmented to also directly call ua_txn->get_proxy_ssn()->release()

For HTTP2,
* Http2Stream::release really just calls Http2Stream::do_io_close. This PR moves the super_type::release() (which calls the decrement) and moves it into Http2Stream::destory(). The original logic would double decrement in do_io_close and release.
* Http2ClientStream::release() is a no-op (unchanged by this PR)

For HTTP1,
* Http1ClientTransaction::release() does nothing. The super_type::release() is called explicitly in transaction_done(). The extra Session specific logic that was in Http1ClientTransaction::release() is moved to Http1ClientSession::release(), which seems more appropriate anyway. There was a comment about the _sm possibly not being available in Http1ClientSession::release, but that no longer seems to be the case.

@maskit could you give me some direction on what we need to fix up Http3? I assume it would be similar to the changes in HTTP2, but there are multiple sessions in HTTP3, so I am a bit unsure which session to review.

Ultimately ProxyTransaction::release() should probably be renamed to something else. Calling super_type::release() seems somewhat obscure.

I have run the tests locally with this commit successfully multiple times. Currently running a test build on a production box and the statistics seem much more sensible.

```
-bash-4.2$ traffic_ctl metric match proxy.process.http.current_client_
NOTE: using the environment variable TS_RUNROOT
proxy.process.http.current_client_connections 12291
proxy.process.http.current_client_transactions 135
-bash-4.2$ traffic_ctl metric match proxy.process.http2.current_client_
NOTE: using the environment variable TS_RUNROOT
proxy.process.http2.current_client_connections 28570
proxy.process.http2.current_client_streams 431
```

----------------------------------------------------------------
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] shinrich commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-706223128


   Hmm.  if r != _reader all the time, that is indeed disturbing.


----------------------------------------------------------------
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] maskit commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

maskit commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-705903123

I think adding another line that calls `decrement_client_transactions_stat` is not a good idea. Having multiple paths would make things complicated.

`ProxyTransaction::decrement_client_transactions_stat` is only called from `ProxyTransaction::release`.
`Http1ClientSession::release_transaction()`, which is the one you added the line, is only called from `Http1Transaction::transaction_done`.

If adding the line into `release_transaction` solves the issue, doesn't it mean `ProxyTransaction::release` is not called for the transaction? I think that is the root cause.

This code looks suspicious. When subclass's `release` is called, it should always call superclass's `release`.
https://github.com/apache/trafficserver/blob/718bef406aacdb14564cb111c3ad14cb9bc1466d/proxy/http/Http1Transaction.cc#L42-L46

Even if this works for the stat issue, a risk for not running common processes on releasing transaction would be left. Since it's a blocker for 9.0, I didn't make a change request. However, I'm not so happy with this approach.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] shinrich commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-707885016


   We do need an autest here to track these 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] shinrich edited a comment on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich edited a comment on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-707883767

Pushed a new more through version of a fix. Looking into this more deeply the Http2 version of this metric was also incorrect, over decrementing instead of under decrementing.

For HTTP2,
* Http2Stream::release really just calls Http2Stream::do_io_close. This PR moves the super_type::release() (which calls the decrement) into Http2Stream::destory(). The original logic would double decrement in do_io_close and release.
* Http2ClientStream::release() is a no-op (unchanged by this PR)

Ultimately ProxyTransaction::release() should probably be renamed to something else. Calling super_type::release() seems somewhat obscure.

I have run the tests locally with this commit successfully multiple times. Currently running a test build on a production box and the statistics seem much more sensible.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] maskit commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

maskit commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-705903123

I think adding another line that calls `decrement_client_transactions_stat` is not a good idea. Having multiple paths would make things complicated.

If adding the line into `release_transaction` solves the issue, doesn't it mean `ProxyTransaction::release` is not called for the transaction? I think that is the root cause.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] shinrich commented on pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich commented on pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258#issuecomment-708400871


   Pushed another commit.  After running the previous box overnight, the http2 current stream count was stuck again at 0 which implies that path was still over decrementing.  I assume that destroy is being called in cases were the stream object created but new_transaction is never called.  So moved that logic into transaction_done as it is for http1.  That kind of lines up with your suggestion @maskit that we push this logic up into the same place for all versions.  So a super class transaction_done method might do it.
   
   Will go ahead and push up another commit with HTTP/3 addressed.


----------------------------------------------------------------
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] shinrich merged pull request #7258: Fix proxy.process.http.current_client_transactions

Posted by GitBox <gi...@apache.org>.

shinrich merged pull request #7258:
URL: https://github.com/apache/trafficserver/pull/7258


   


----------------------------------------------------------------
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