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

[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.
   
   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) 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