add a metric for closed connections #2676
Conversation
| case logging.TimeoutReasonIdle: | ||
| return "idle_timeout" | ||
| default: | ||
| panic("unknown timeout reason") |
There was a problem hiding this comment.
Don't panic, just return "unknown". Metrics is never a good enough reason to crash a program.
There was a problem hiding this comment.
I tend to agree, although if we don't panic, those bugs will probably go unnoticed for a long time (until someone looks at the logs and sees a bunch of unknowns there). Anyway, this is a larger change, since we currently panic both in the logging and in the qlog package.
| @@ -106,7 +117,33 @@ func (t *connTracer) StartedConnection(local, _ net.Addr, _ logging.VersionNumbe | |||
| ) | |||
| } | |||
|
|
|||
| func (t *connTracer) ClosedConnection(logging.CloseReason) {} | |||
| func (t *connTracer) ClosedConnection(r logging.CloseReason) { | |||
| var tags []tag.Mutator | |||
There was a problem hiding this comment.
I wonder whether this could be simplified by doing more of this processing during the CloseReason creation already. For example the type of reason, or whether it's remote or not could be set when creating the CloseReason, rather than interpreting errors later on. Essentially, make CloseReason POD?
There was a problem hiding this comment.
Yes, CloseReason is a mess. I'd like to clean this up, but I'm not really sure how.
marten-seemann
force-pushed
the
conn-close-metric
branch
from
bb17358
to
8a21cf7
Compare
Codecov Report
@@ Coverage Diff @@
## master #2676 +/- ##
==========================================
+ Coverage 86.73% 86.82% +0.09%
==========================================
Files 124 124
Lines 9878 9968 +90
==========================================
+ Hits 8567 8654 +87
- Misses 977 978 +1
- Partials 334 336 +2
Continue to review full report at Codecov.
|
I'm not really happy with this PR. This seems overly complicated (and converting a bool to string is not pretty either).
The reason this is so complicated is that there are many reasons a QUIC connection can be closed:
@lanzafame Does this look ok to you?