Fix/sentinel filter #2621
Fix/sentinel filter #2621
Conversation
…d, and sentinel.InitDefault() not being called
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2621 +/- ##
=======================================
Coverage 47.44% 47.44%
=======================================
Files 341 341
Lines 25168 25168
=======================================
Hits 11940 11940
Misses 12084 12084
Partials 1144 1144 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks! A few comments about code style.
In addition, I have another concern: sentinel.TraceError is called by OnResponse() previously, but now is called by Invoke(). THE SIDE EFFECT: You changes the behavior of the filter.
IMHO, your concern is that the context isn't updated with the latest data, right? So a possible fix could be:
func (fi *FilterInvoker) Invoke(ctx context.Context, invocation protocol.Invocation) protocol.Result {
result := fi.filter.Invoke(ctx, fi.next, invocation)
// do something to copy the context from the latest one
return fi.filter.OnResponse(ctx, result, fi.invoker, invocation)
} Wdyt?
UPDATE 1: Maybe the above way is not easy to implement. But the main idea is to share the context between fi.filter.Invoke() and fi.filter.OnResponse().
filter/sentinel/filter.go
Outdated
| err := sentinel.InitDefault() | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
| err := sentinel.InitDefault() | |
| if err != nil { | |
| panic(err) | |
| } | |
| if err := sentinel.InitDefault(); err != nil { | |
| panic(err) | |
| } |
filter/sentinel/filter.go
Outdated
| } else { | ||
| defer interfaceEntry.Exit() | ||
| } |
There was a problem hiding this comment.
else here could be removed.
| } else { | |
| defer interfaceEntry.Exit() | |
| } | |
| } | |
| defer interfaceEntry.Exit() |
filter/sentinel/filter.go
Outdated
| } else { | ||
| defer methodEntry.Exit() | ||
| } |
filter/sentinel/filter.go
Outdated
| } else { | ||
| defer interfaceEntry.Exit() | ||
| } |
filter/sentinel/filter.go
Outdated
| } else { | ||
| defer methodEntry.Exit() | ||
| } |
I also wanted to share context between Perhaps there will never be a need to perform some operations between |
There was a problem hiding this comment.
This looks good to me overall. For what I mentioned eariler, I found a post introducing a wrapped context, which could be worth considering.
|
* fix: the bug of sentinel.TraceError and sentinel.Exit not being called, and sentinel.InitDefault() not being called * test: add test sentinel filter error count * fix: The problem of sentinel.TraceError not being called in sentinelConsumerFilter. * style: adjust code style.
#2620