feat: return entry when request is blocked. #494
Conversation
|
|
|
@binbin0325 Seems like this golanglint-ci should be upgraded, and there are many lint errors should be resolved. And this PR should have no affects about the failed CI. $ golangci-lint --version golangci-lint run
core/hotspot/slot_test.go:48:2: S1023: redundant `return` statement (gosimple)
return
^
core/hotspot/traffic_shaping_test.go:35:2: S1023: redundant `return` statement (gosimple)
return
^
core/hotspot/traffic_shaping_test.go:78:2: S1023: redundant `return` statement (gosimple)
return
^
core/hotspot/rule.go:104:25: SA1026: trying to marshal unsupported type map[interface{}]int64, via x.SpecificItems (staticcheck)
b, err := json.Marshal(r)
^
core/base/result_test.go:49:19: Error return value is not checked (errcheck)
RegistryBlockType(BlockTypeNew1, "new1")
^
core/base/result_test.go:87:2: var `blockTypeErr` is unused (unused)
blockTypeErr = fmt.Errorf("block type err")
^
core/stat/base/leap_array.go:46:23: func `(*BucketWrap).isTimeInBucket` is unused (unused)
func (ww *BucketWrap) isTimeInBucket(now uint64, bucketLengthInMs uint32) bool {
^
core/stat/base/leap_array.go:42:23: func `(*BucketWrap).resetTo` is unused (unused)
func (ww *BucketWrap) resetTo(startTime uint64) {
^
logging/logging.go:217:11: S1034: assigning the result of this type assertion to a variable (switch v := v.(type)) could eliminate type assertions in switch cases (gosimple)
switch v.(type) {
^
logging/logging.go:219:30: S1034(related information): could eliminate this type assertion (gosimple)
data := toSafeJSONString(v.(string))
^
logging/logging.go:222:30: S1034(related information): could eliminate this type assertion (gosimple)
data := toSafeJSONString(v.(error).Error())
^
util/atomic.go:47:2: S1008: should use 'return atomic.LoadInt32(&(b.flag)) != 0' instead of 'if atomic.LoadInt32(&(b.flag)) != 0 { return true }; return false' (gosimple)
if atomic.LoadInt32(&(b.flag)) != 0 {
^
core/isolation/rule_manager.go:192:2: S1011: should replace loop with `ret = append(ret, resRules...)` (gosimple)
for _, r := range resRules {
^
core/system_metric/sys_metric_stat.go:77:26: Error return value of `metric_exporter.Register` is not checked (errcheck)
metric_exporter.Register(cpuRatioGauge)
^
core/system_metric/sys_metric_stat.go:78:26: Error return value of `metric_exporter.Register` is not checked (errcheck)
metric_exporter.Register(processMemoryGauge)
^
core/stat/stat_slot.go:39:26: Error return value of `metric_exporter.Register` is not checked (errcheck)
metric_exporter.Register(handledCounter)
^
ext/datasource/helper_test.go:108:18: Error return value of `flow.ClearRules` is not checked (errcheck)
flow.ClearRules()
^
ext/datasource/helper_test.go:109:17: Error return value of `flow.LoadRules` is not checked (errcheck)
flow.LoadRules([]*flow.Rule{
^
ext/datasource/helper_test.go:126:18: Error return value of `flow.ClearRules` is not checked (errcheck)
flow.ClearRules()
^
ext/datasource/helper_test.go:132:18: Error return value of `flow.ClearRules` is not checked (errcheck)
flow.ClearRules()
^
ext/datasource/helper_test.go:216:20: Error return value of `system.ClearRules` is not checked (errcheck)
system.ClearRules()
^
ext/datasource/helper_test.go:217:19: Error return value of `system.LoadRules` is not checked (errcheck)
system.LoadRules([]*system.Rule{
^
ext/datasource/helper_test.go:230:20: Error return value of `system.ClearRules` is not checked (errcheck)
system.ClearRules()
^
ext/datasource/helper_test.go:236:20: Error return value of `system.ClearRules` is not checked (errcheck)
system.ClearRules()
^
ext/datasource/file/refreshable_file_test.go:250:16: Error return value of `f.WriteString` is not checked (errcheck)
f.WriteString("\n" + TestSystemRules)
^
ext/datasource/file/refreshable_file_test.go:251:9: Error return value of `f.Sync` is not checked (errcheck)
f.Sync()
^
tests/benchmark/hotspot/hotspot_benchmark_test.go:40:2: var `hotspotParamValue` is unused (unused)
hotspotParamValue = "hotspotParamValue"
^
core/log/metric/aggregator.go:78:2: S1000: should use for range instead of for { select {} } (gosimple)
for {
^
core/circuitbreaker/circuit_breaker.go:63:2: S1021: should merge variable declaration with assignment on next line (gosimple)
var state State
^
core/log/metric/reader.go:193:5: S1009: should omit nil check; len() for []*github.com/alibaba/sentinel-golang/core/base.MetricItem is defined as zero (gosimple)
if items == nil || len(items) == 0 {
^ |
|
Run the command in the go.yml $ golangci-lint run --timeout=10m -v --disable-all --enable=govet --enable=staticcheck --enable=ineffassign --enable=misspell core/hotspot/rule.go:104:25: SA1026: trying to marshal unsupported type map[interface{}]int64, via x.SpecificItems (staticcheck)
b, err := json.Marshal(r) |
|
Refer this, the failed CI is caused by x/sys, which use unsafe.Slice new from go1.17.x We should consider to upgrade go mod to 1.18 or above, and it's done in this PR. |
Codecov ReportBase: 53.17% // Head: 53.16% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #494 +/- ##
==========================================
- Coverage 53.17% 53.16% -0.01%
==========================================
Files 91 91
Lines 5896 5889 -7
==========================================
- Hits 3135 3131 -4
+ Misses 2414 2412 -2
+ Partials 347 346 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| diff -u <(echo -n) <(gofmt -d -s .) | ||
| diff -u <(echo -n) <(goimports -d .) |
There was a problem hiding this comment.
this two line will help to locate code which needs to be re-format
| - name: Install goimports | ||
| run: go get golang.org/x/tools/cmd/goimports |
There was a problem hiding this comment.
goimports check is necessary to format code
There was a problem hiding this comment.
It's recommend to use linters in golangci-lint.
| return fmt.Sprintf("{Id:%s, Resource:%s, MetricType:%+v, ControlBehavior:%+v, ParamIndex:%d, ParamKey:%s, Threshold:%d, MaxQueueingTimeMs:%d, BurstCount:%d, DurationInSec:%d, ParamsMaxCapacity:%d, SpecificItems:%+v}", | ||
| r.ID, r.Resource, r.MetricType, r.ControlBehavior, r.ParamIndex, r.ParamKey, r.Threshold, r.MaxQueueingTimeMs, r.BurstCount, r.DurationInSec, r.ParamsMaxCapacity, r.SpecificItems) |
There was a problem hiding this comment.
maybe it is more appropriate to keep the original
There was a problem hiding this comment.
json marshal do not support map[interface{}]int64 like SpecificItems
| @@ -176,7 +176,7 @@ func entry(resource string, options *EntryOptions) (*base.SentinelEntry, *base.B | |||
| // must finish the lifecycle of r. | |||
| blockErr := base.NewBlockErrorFromDeepCopy(r.BlockError()) | |||
| e.Exit() | |||
| return nil, blockErr | |||
| return e, blockErr | |||
There was a problem hiding this comment.
Why return entry when blocked, and what do you need to do with it
There was a problem hiding this comment.
Leave the control to caller, and they can do any thing like log/metrics/fallback which needs entry info.
There was a problem hiding this comment.
I don't understand. Do you want to get an EntryContext through entry? or you can give an example, how do you do things like log/metric/fallback
There was a problem hiding this comment.
Here is the use case. Right now, there is no way to fetch entry info like ResourceType/TrafficType... when error happens.
This pr aims to return entry object when error returned, and other changes are caused by the origin master code which can not pass the ci process :-) |
Describe what this PR does / why we need it
Does this pull request fix one issue?
Fixed #493
Describe how you did it
Describe how to verify it
Special notes for reviews