Skip to content

transport: Increment metrics only when the stream is active - #8573

Merged
easwars merged 14 commits into
grpc:masterfrom
hugehoo:fix-unneccesary-metric-emits
Oct 7, 2025
Merged

transport: Increment metrics only when the stream is active#8573
easwars merged 14 commits into
grpc:masterfrom
hugehoo:fix-unneccesary-metric-emits

Conversation

@hugehoo

@hugehoo hugehoo commented Sep 11, 2025 •
edited by arjan-bal
Loading

Copy link
Copy Markdown
Contributor

Fixes: #8529

This PR fixes to increment metrics only when the stream is active which is found in the activeStreams map.

as-is

  • The deleteStream was incrementing channelz metrics every time it was called, even when stream was already removed from activeStreams or not exists in activeStreams.

to-be

  • Added check to ensure metrics are only incremented once when a stream is actually removed from activeStreams.

RELEASE NOTES:

  • server: Fix a bug that caused overcounting of channelz metrics for successful and failed streams.
@codecov

codecov Bot commented Sep 11, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.13%. Comparing base (8420f3f) to head (7636f6e).
⚠️ Report is 37 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8573      +/-   ##
==========================================
+ Coverage   80.91%   82.13%   +1.21%     
==========================================
  Files         413      415       +2     
  Lines       40751    40712      -39     
==========================================
+ Hits        32972    33437     +465     
+ Misses       6155     5899     -256     
+ Partials     1624     1376     -248
Files with missing lines Coverage Δ
internal/transport/http2_server.go 90.88% <100.00%> (+<0.01%) ⬆️

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@hugehoo
hugehoo marked this pull request as ready for review September 11, 2025 17:41
@arjan-bal
arjan-bal self-requested a review September 23, 2025 05:05
@arjan-bal arjan-bal self-assigned this Sep 23, 2025
@arjan-bal arjan-bal added this to the 1.77 Release milestone Sep 23, 2025
arjan-bal
arjan-bal reviewed Sep 23, 2025
View reviewed changes

@arjan-bal arjan-bal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending the PR, nice job writing a unit test for this!
Comment thread internal/transport/http2_server.go Outdated
Comment thread internal/transport/transport_test.go Outdated
Comment thread internal/transport/transport_test.go Outdated
Comment thread internal/transport/transport_test.go Outdated
Comment thread internal/transport/transport_test.go Outdated
@arjan-bal arjan-bal assigned hugehoo and unassigned arjan-bal Sep 23, 2025
@hugehoo
hugehoo requested a review from arjan-bal September 29, 2025 16:11
@arjan-bal arjan-bal assigned arjan-bal and unassigned hugehoo Sep 29, 2025
arjan-bal
arjan-bal reviewed Oct 3, 2025
View reviewed changes
Comment thread internal/transport/transport_test.go Outdated
Comment thread internal/transport/transport_test.go Outdated
Comment thread internal/transport/transport_test.go Outdated
@arjan-bal arjan-bal assigned hugehoo and unassigned arjan-bal Oct 3, 2025
@hugehoo

hugehoo commented Oct 3, 2025

Copy link
Copy Markdown
Contributor Author

@arjan-bal thx for the review, i updated additional comments
@hugehoo
hugehoo requested a review from arjan-bal October 3, 2025 11:54
@arjan-bal arjan-bal assigned arjan-bal and unassigned hugehoo Oct 6, 2025
arjan-bal
arjan-bal approved these changes Oct 6, 2025
View reviewed changes

@arjan-bal arjan-bal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with a minor comment. Assigning a second reviewer.
Comment thread internal/transport/transport_test.go Outdated
}

// Always create a real stream through the client
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)

@arjan-bal arjan-bal Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We should create a context at the beginning of t.Run with a timeout of defaultTestTimeout. This should help ensure that the entire test case finishes within 10 seconds.

@hugehoo hugehoo Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated here 7636f6e
thx for review !
@arjan-bal arjan-bal removed their assignment Oct 6, 2025
@arjan-bal
arjan-bal requested a review from easwars October 6, 2025 08:45
easwars
easwars approved these changes Oct 6, 2025
View reviewed changes
Comment thread internal/transport/transport_test.go Outdated
@easwars easwars removed their assignment Oct 6, 2025
@easwars

easwars commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

@hugehoo : Please take care of the couple of minor nitty comments. Thanks for your contribution again.
@easwars

easwars commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

Also, the PR title seems to be broken. Could you please fix that too.
@hugehoo hugehoo changed the title transport: add metrics when it s active Oct 7, 2025
@hugehoo
hugehoo requested review from arjan-bal and easwars October 7, 2025 08:20
@easwars
easwars merged commit 9ff80a7 into grpc:master Oct 7, 2025
16 checks passed
@arjan-bal arjan-bal mentioned this pull request Oct 14, 2025
Closed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Type: Bug

3 participants

@hugehoo @easwars @arjan-bal