Skip to content

Commit

Permalink
fix: SlowThreshold configuration not taking effect (#4654)
Browse files Browse the repository at this point in the history
  • Loading branch information
Nanosk07 authored Feb 14, 2025
1 parent 560c616 commit 6a0672b
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 33 deletions.
7 changes: 3 additions & 4 deletions zrpc/internal/serverinterceptors/statinterceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ var (

// StatConf defines the static configuration for stat interceptor.
type StatConf struct {
SlowThreshold time.Duration `json:",default=500ms"`
SlowThreshold time.Duration `json:",optional"`
IgnoreContentMethods []string `json:",optional"`
}

Expand Down Expand Up @@ -63,8 +63,7 @@ func UnaryStatInterceptor(metrics *stat.Metrics, conf StatConf) grpc.UnaryServer
}

func isSlow(duration, durationThreshold time.Duration) bool {
return duration > slowThreshold.Load() ||
(durationThreshold > 0 && duration > durationThreshold)
return durationThreshold > 0 && duration > durationThreshold
}

func logDuration(ctx context.Context, method string, req any, duration time.Duration,
Expand All @@ -84,7 +83,7 @@ func logDuration(ctx context.Context, method string, req any, duration time.Dura
content, err := json.Marshal(req)
if err != nil {
logx.WithContext(ctx).Errorf("%s - %s", addr, err.Error())
} else if duration > slowThreshold.Load() {
} else if isSlow(duration, durationThreshold) {
logger.Slowf("[RPC] slowcall - %s - %s - %s", addr, method, string(content))
} else {
logger.Infof("%s - %s - %s", addr, method, string(content))
Expand Down
52 changes: 23 additions & 29 deletions zrpc/internal/serverinterceptors/statinterceptor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,11 @@ func TestLogDuration(t *testing.T) {
assert.True(t, len(addrs) > 0)

tests := []struct {
name string
ctx context.Context
req any
duration time.Duration
name string
ctx context.Context
req any
duration time.Duration
durationThreshold time.Duration
}{
{
name: "normal",
Expand All @@ -59,10 +60,11 @@ func TestLogDuration(t *testing.T) {
req: make(chan lang.PlaceholderType), // not marshalable
},
{
name: "timeout",
ctx: context.Background(),
req: "foo",
duration: time.Second,
name: "timeout",
ctx: context.Background(),
req: "foo",
duration: time.Second,
durationThreshold: time.Millisecond * 500,
},
{
name: "timeout",
Expand All @@ -86,7 +88,7 @@ func TestLogDuration(t *testing.T) {

assert.NotPanics(t, func() {
logDuration(test.ctx, "foo", test.req, test.duration,
collection.NewSet(), 0)
collection.NewSet(), test.durationThreshold)
})
})
}
Expand All @@ -98,10 +100,11 @@ func TestLogDurationWithoutContent(t *testing.T) {
assert.True(t, len(addrs) > 0)

tests := []struct {
name string
ctx context.Context
req any
duration time.Duration
name string
ctx context.Context
req any
duration time.Duration
durationThreshold time.Duration
}{
{
name: "normal",
Expand All @@ -114,10 +117,11 @@ func TestLogDurationWithoutContent(t *testing.T) {
req: make(chan lang.PlaceholderType), // not marshalable
},
{
name: "timeout",
ctx: context.Background(),
req: "foo",
duration: time.Second,
name: "timeout",
ctx: context.Background(),
req: "foo",
duration: time.Second,
durationThreshold: time.Millisecond * 500,
},
{
name: "timeout",
Expand Down Expand Up @@ -146,7 +150,7 @@ func TestLogDurationWithoutContent(t *testing.T) {

assert.NotPanics(t, func() {
logDuration(test.ctx, "foo", test.req, test.duration,
collection.NewSet(), 0)
collection.NewSet(), test.durationThreshold)
})
})
}
Expand Down Expand Up @@ -225,7 +229,7 @@ func Test_isSlow(t *testing.T) {
args{
duration: time.Millisecond * 501,
},
true,
false,
nil,
},
{
Expand All @@ -237,16 +241,6 @@ func Test_isSlow(t *testing.T) {
true,
nil,
},
{
"dynamic",
args{
duration: time.Millisecond * 200,
},
true,
func() {
SetSlowThreshold(time.Millisecond * 100)
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down

0 comments on commit 6a0672b

Please sign in to comment.