Skip to content

FdSocket: Count TLS handshake stats during state transitions.#1125

Merged
prbprbprb merged 1 commit into
google:masterfrom
prbprbprb:fdsocket_stats
Apr 3, 2023
Merged

FdSocket: Count TLS handshake stats during state transitions.#1125
prbprbprb merged 1 commit into
google:masterfrom
prbprbprb:fdsocket_stats

Conversation

@prbprbprb

Copy link
Copy Markdown
Contributor

This is analogous to #1120 and removes double-counting by only counting metrics during explicit state changes. There is a lot more tidying up that could be done here, but as this class is long deprecated, this change is kept to the minimum required to ensure consistent counting between the EngineSocket and FdSocket.

Also incorporates the "failed" values from #1123 to make analysis clearer.

TODO(prb): Tested manually: We have no real tests to ensure consistent stats counting.

This is analogous to google#1120 and removes double-counting by only
counting metrics during explicit state changes.  There is a lot
more tidying up that could be done here, but as this class is
long deprecated, this change is kept to the minimum required to
ensure consistent counting between the EngineSocket and FdSocket.

Also incorporates the "failed" values from google#1123 to make analysis
clearer.

TODO(prb): Tested manually: We have no real tests to ensure
consistent stats counting.
@prbprbprb prbprbprb requested review from Yqwed and miguelaranda0 April 2, 2023 11:10
@prbprbprb

Copy link
Copy Markdown
Contributor Author

NB One difference between this and #1120 is that despite what the comments say, FdSocket never actually transitions through STATE_READY_HANDSHAKE_CUT_THROUGH, instead it jumps straight to STATE_READY in the ssl callback and runs the handshake listeners in that state. But again, no point fixing that in its deprecated state.

@prbprbprb prbprbprb merged commit 82741c4 into google:master Apr 3, 2023
@prbprbprb prbprbprb deleted the fdsocket_stats branch May 23, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants