Skip to content

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

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

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

Conversation

@hugehoo
Copy link
Copy Markdown
Contributor

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

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
Copy link
Copy Markdown

codecov Bot commented Sep 11, 2025 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

@arjan-bal arjan-bal left a comment

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
Copy link
Copy Markdown
Contributor Author

hugehoo commented Oct 3, 2025

@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
Copy link
Copy Markdown
Contributor

@arjan-bal arjan-bal left a comment

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)
Copy link
Copy Markdown
Contributor

@arjan-bal arjan-bal Oct 6, 2025

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.

Copy link
Copy Markdown
Contributor Author

@hugehoo hugehoo Oct 7, 2025

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
Copy link
Copy Markdown
Contributor

easwars commented Oct 6, 2025

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

@easwars
Copy link
Copy Markdown
Contributor

easwars commented Oct 6, 2025

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 transport: Increment metrics only when the stream is 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.

Reviewers

@easwars easwars Awaiting requested review from easwars

@arjan-bal arjan-bal Awaiting requested review from arjan-bal

Assignees

@hugehoo hugehoo

Labels

Type: Bug

Projects

None yet

Milestone

1.77 Release

Development

Successfully merging this pull request may close these issues.

transport: Servers may emit extra channelz stream metrics on cancellation

3 participants

@hugehoo @easwars @arjan-bal