Expose inbound.cancels.{requested,honored} metrics
#912
Conversation
|
|
|
cc: @prashantv |
|
Note that I couldn't run all the tests locally, because of the missing > make test
<...>
scripts/install-thrift.sh /Users/vilius/go/src/github.com/vpranckaitis/tchannel-go/.bin
--2024-01-17 09:25:27-- https://github.com/uber/tchannel-go/releases/download/thrift-v1.0.0-dev/thrift-1-darwin-arm64.tar.gz
Resolving github.com (github.com)... 140.82.121.4
Connecting to github.com (github.com)|140.82.121.4|:443... connected.
HTTP request sent, awaiting response... 404 Not Found
2024-01-17 09:25:27 ERROR 404: Not Found. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #912 +/- ##
==========================================
- Coverage 89.25% 89.19% -0.07%
==========================================
Files 43 43
Lines 4515 4517 +2
==========================================
- Hits 4030 4029 -1
+ Misses 370 369 -1
- Partials 115 119 +4 ☔ View full report in Codecov by Sentry. |
connection_test.go
Outdated
| @@ -519,16 +522,22 @@ func TestServerClientCancellation(t *testing.T) { | |||
| _, _, _, err := raw.Call(ctx, ts.Server(), ts.HostPort(), ts.ServiceName(), "ctxWait", nil, nil) | |||
| assert.Equal(t, ErrRequestCancelled, err, "client call result") | |||
|
|
|||
| serverStats.Expected.IncCounter("inbound.cancels.requested", ts.Server().StatsTags(), 1) | |||
There was a problem hiding this comment.
nit: .StatsTags() allocates, so should call just once (not a big deal in tests, but still good practice)
connection_test.go
Outdated
| @@ -568,6 +581,13 @@ func TestCancelWithoutSendCancelOnContextCanceled(t *testing.T) { | |||
| _, _, _, err := raw.Call(ctx, ts.Server(), ts.HostPort(), ts.ServiceName(), "ctxWait", nil, nil) | |||
| assert.Equal(t, ErrRequestCancelled, err, "client call result") | |||
|
|
|||
| if tt.wantCancelRequested { | |||
| serverStats.Expected.IncCounter("inbound.cancels.requested", ts.Server().StatsTags(), 1) | |||
There was a problem hiding this comment.
are we missing a call to serverStats.ValidateExpected(t) below?
There was a problem hiding this comment.
Yes, it's missing the ValidateExpected(). However, tried adding it below and the test wouldn't pass, because:
- the
with_relaysubtest insideWithTestServerdoesn't propagate the cancellation, thus the metric is never incremented; - the
no_relaysubtest insdeWithTestServerincreases the metric, but it happens not immediately upon cancel, only after the function passed intoWithTestServeris finished executing.
I'm not yet sure how to solve this cleanly. I will dig deeper into this once I have more time.
There was a problem hiding this comment.
This seems to be working (93cd04f). Please feel free to suggest a different approach.
|
bump? |
|
@cinchurge there was a Lint failure due to missing comment for a method. I've added it in e39a740, need to rerun the CI. |
### Added
* Expose `inbound.cancels.{requested,honored}` metrics (#912)
This commit adds
inbound.cancels.requestedandinbound.cancels.honoredmetrics. The documentation mentions these metrics, but they were not yet implemented.