transport: add metrics when it s active #8573
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8573 +/- ##
==========================================
+ Coverage 80.91% 82.20% +1.29%
==========================================
Files 413 415 +2
Lines 40751 40712 -39
==========================================
+ Hits 32972 33467 +495
+ Misses 6155 5877 -278
+ Partials 1624 1368 -256
🚀 New features to boost your workflow:
|
internal/transport/transport_test.go
Outdated
| if test.removeFromActive { | ||
| serverTransport.mu.Lock() | ||
| delete(serverTransport.activeStreams, serverStream.id) | ||
| serverTransport.mu.Unlock() | ||
| } |
There was a problem hiding this comment.
I think we should not access the activeStreams map directly. We should rely on the first call to serverTransport.deleteStream to delete the stream. After the first call to serverTransport.deleteStream, we should assert that the metric counts are as expected.
After the first call to serverTransport.deleteStream, we can assume that the stream is deleted from the activeStreams map. We can call serverTransport.deleteStream a couple of times more and verify that the metric values haven't changed.
This would allow us to get rid of the removeFromActive test param since that case is already covered by the assertions made in the previous paragraph.
internal/transport/transport_test.go
Outdated
| if !test.removeFromActive { | ||
| serverTransport.mu.Lock() | ||
| _, exists := serverTransport.activeStreams[serverStream.id] | ||
| serverTransport.mu.Unlock() | ||
| if exists { | ||
| t.Error("Stream should have been removed from activeStreams") | ||
| } | ||
| } |
There was a problem hiding this comment.
We can remove the assertions on the activeStreams map here. Ideally we should test the public API of a struct (methods on serverTransport) instead of it's internal details. In this case, testing the metric should give us enough confidence that the state of the map is correct.
internal/transport/transport_test.go
Outdated
| initialSucceeded := serverTransport.channelz.SocketMetrics.StreamsSucceeded.Load() | ||
| initialFailed := serverTransport.channelz.SocketMetrics.StreamsFailed.Load() |
There was a problem hiding this comment.
These values will always be 0, right? If so, we can assert they are 0 and assert that the absolute values of the metrics later instead of deltas. This would make it clearer for readers to see exactly what is the expected value of the metric.
|
@arjan-bal thx for the review, i updated additional comments |
Fixes: #8529
This PR fixes to increment metrics only when the stream is active which is found in the activeStreams map.
as-is
to-be
RELEASE NOTES: N/A