Skip to content

chore: refactor canSkipMethods logic #803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 22, 2024
Merged

chore: refactor canSkipMethods logic #803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions api/internal/consumer/consumer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ type Consumer struct {
FilterConfigs map[string]*fmModel.ParsedFilterConfig

// fields that generated from the configuration
FilterNames []string
InitOnce sync.Once
CanSkipMethod map[string]bool
CanSkipMethodOnce sync.Once
CanSyncRunMethod map[string]bool
FilterNames []string
InitOnce sync.Once
CanSkipMethods map[string]bool
CanSkipMethodsOnce sync.Once
CanSyncRunMethod map[string]bool
// CanSyncRunMethod share the same sync.Once with CanSkipMethodOnce
}

Expand Down
14 changes: 14 additions & 0 deletions api/pkg/filtermanager/api/phase.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,17 @@ func MethodToPhase(meth string) Phase {
return 0
}
}

func NewAllMethodsMap() map[string]bool {
return map[string]bool{
"DecodeHeaders": true,
"DecodeData": true,
"DecodeRequest": true,
"DecodeTrailers": true,
"EncodeHeaders": true,
"EncodeData": true,
"EncodeResponse": true,
"EncodeTrailers": true,
"OnLog": true,
}
}
135 changes: 54 additions & 81 deletions api/pkg/filtermanager/filtermanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@
canSkipEncodeData bool
canSkipEncodeTrailers bool
canSkipOnLog bool
canSkipMethod map[string]bool
canSkipMethods map[string]bool

canSyncRunDecodeHeaders bool
canSyncRunDecodeData bool
canSyncRunDecodeTrailers bool
canSyncRunEncodeHeaders bool
canSyncRunEncodeData bool
canSyncRunEncodeTrailers bool
canSyncRunMethod map[string]bool
canSyncRunMethods map[string]bool

callbacks *filterManagerCallbackHandler
config *filterManagerConfig
Expand Down Expand Up @@ -94,14 +94,15 @@
m.canSkipEncodeData = false
m.canSkipEncodeTrailers = false
m.canSkipOnLog = false
// m.canSkipMethods is reused across filters in the same config

m.canSyncRunDecodeHeaders = false
m.canSyncRunDecodeData = false
m.canSyncRunDecodeTrailers = false
m.canSyncRunEncodeHeaders = false
m.canSyncRunEncodeData = false
m.canSyncRunEncodeTrailers = false
// m.canSyncRunMethod is reused across filters in the same config
// m.canSyncRunMethods is reused across filters in the same config

m.callbacks.Reset()
}
Expand All @@ -122,33 +123,6 @@
return m.config.enableDebugMode
}

func newSkipMethodsMap() map[string]bool {
return map[string]bool{
"DecodeHeaders": true,
"DecodeData": true,
"DecodeRequest": true,
"DecodeTrailers": true,
"EncodeHeaders": true,
"EncodeData": true,
"EncodeResponse": true,
"EncodeTrailers": true,
"OnLog": true,
}
}

func newSyncRunMethodMap() map[string]bool {
return map[string]bool{
"DecodeHeaders": true,
"DecodeData": true,
"DecodeRequest": true,
"DecodeTrailers": true,
"EncodeHeaders": true,
"EncodeData": true,
"EncodeResponse": true,
"EncodeTrailers": true,
}
}

func needLogExecution() bool {
return api.GetLogLevel() <= api.LogLevelDebug
}
Expand Down Expand Up @@ -179,11 +153,11 @@

fm.callbacks.FilterCallbackHandler = cb

canSkipMethod := fm.canSkipMethod
canSyncRunMethod := fm.canSyncRunMethod
if canSkipMethod == nil {
canSkipMethod = newSkipMethodsMap()
canSyncRunMethod = newSyncRunMethodMap()
canSkipMethods := fm.canSkipMethods
canSyncRunMethods := fm.canSyncRunMethods
if canSkipMethods == nil {
canSkipMethods = api.NewAllMethodsMap()
canSyncRunMethods = api.NewAllMethodsMap()
}

filters := make([]*model.FilterWrapper, len(parsedConfig))
Expand All @@ -193,23 +167,22 @@
config := fc.ParsedConfig
f := factory(config, fm.callbacks)
// Technically, the factory might create different f for different calls. We don't support this edge case for now.
if fm.canSkipMethod == nil {
definedMethod := make(map[string]bool, len(canSkipMethod))
for meth := range canSkipMethod {
if fm.canSkipMethods == nil {
definedMethod := make(map[string]bool, len(canSkipMethods))
for meth := range canSkipMethods {
definedMethod[meth] = false
}
for meth := range canSkipMethod {
for meth := range canSkipMethods {
overridden, err := reflectx.IsMethodOverridden(f, meth)
if err != nil {
api.LogErrorf("failed to check method %s in plugin %s: %v", meth, fc.Name, err)
// canSkipMethod[meth] will be false
// canSkipMethods[meth] will be false

Check warning on line 179 in api/pkg/filtermanager/filtermanager.go

View check run for this annotation

Codecov / codecov/patch

api/pkg/filtermanager/filtermanager.go#L179 

Added line #L179 was not covered by tests
}
canSkipMethod[meth] = canSkipMethod[meth] && !overridden
canSkipMethods[meth] = canSkipMethods[meth] && !overridden
definedMethod[meth] = overridden

if overridden {
// canSkipMethod contains canSyncRunMethod so we can safely check it in the same loop
canSyncRunMethod[meth] = canSyncRunMethod[meth] && fc.SyncRunPhases.Contains(api.MethodToPhase(meth))
canSyncRunMethods[meth] = canSyncRunMethods[meth] && fc.SyncRunPhases.Contains(api.MethodToPhase(meth))
}
}

Expand Down Expand Up @@ -244,31 +217,31 @@
}
}

if fm.canSkipMethod == nil {
fm.canSkipMethod = canSkipMethod
fm.canSyncRunMethod = canSyncRunMethod
if fm.canSkipMethods == nil {
fm.canSkipMethods = canSkipMethods
fm.canSyncRunMethods = canSyncRunMethods
}

// We can't cache the slice of filters as it may be changed by consumer
fm.filters = filters

// The skip check is based on the compiled code. So if the DecodeRequest is defined,
// even it is not called, DecodeData will not be skipped. Same as EncodeResponse.
fm.canSkipDecodeHeaders = fm.canSkipMethod["DecodeHeaders"] && fm.canSkipMethod["DecodeRequest"] && fm.config.initOnce == nil
fm.canSkipDecodeData = fm.canSkipMethod["DecodeData"] && fm.canSkipMethod["DecodeRequest"]
fm.canSkipDecodeTrailers = fm.canSkipMethod["DecodeTrailers"] && fm.canSkipMethod["DecodeRequest"]
fm.canSkipEncodeHeaders = fm.canSkipMethod["EncodeHeaders"]
fm.canSkipEncodeData = fm.canSkipMethod["EncodeData"] && fm.canSkipMethod["EncodeResponse"]
fm.canSkipEncodeTrailers = fm.canSkipMethod["EncodeTrailers"] && fm.canSkipMethod["EncodeResponse"]
fm.canSkipOnLog = fm.canSkipMethod["OnLog"]
fm.canSkipDecodeHeaders = fm.canSkipMethods["DecodeHeaders"] && fm.canSkipMethods["DecodeRequest"] && fm.config.initOnce == nil
fm.canSkipDecodeData = fm.canSkipMethods["DecodeData"] && fm.canSkipMethods["DecodeRequest"]
fm.canSkipDecodeTrailers = fm.canSkipMethods["DecodeTrailers"] && fm.canSkipMethods["DecodeRequest"]
fm.canSkipEncodeHeaders = fm.canSkipMethods["EncodeHeaders"]
fm.canSkipEncodeData = fm.canSkipMethods["EncodeData"] && fm.canSkipMethods["EncodeResponse"]
fm.canSkipEncodeTrailers = fm.canSkipMethods["EncodeTrailers"] && fm.canSkipMethods["EncodeResponse"]
fm.canSkipOnLog = fm.canSkipMethods["OnLog"]

// Similar to the skip check
fm.canSyncRunDecodeHeaders = fm.canSyncRunMethod["DecodeHeaders"] && fm.canSyncRunMethod["DecodeRequest"] && fm.config.initOnce == nil
fm.canSyncRunDecodeData = fm.canSyncRunMethod["DecodeData"] && fm.canSyncRunMethod["DecodeRequest"]
fm.canSyncRunDecodeTrailers = fm.canSyncRunMethod["DecodeTrailers"] && fm.canSyncRunMethod["DecodeRequest"]
fm.canSyncRunEncodeHeaders = fm.canSyncRunMethod["EncodeHeaders"]
fm.canSyncRunEncodeData = fm.canSyncRunMethod["EncodeData"] && fm.canSyncRunMethod["EncodeResponse"]
fm.canSyncRunEncodeTrailers = fm.canSyncRunMethod["EncodeTrailers"] && fm.canSyncRunMethod["EncodeResponse"]
fm.canSyncRunDecodeHeaders = fm.canSyncRunMethods["DecodeHeaders"] && fm.canSyncRunMethods["DecodeRequest"] && fm.config.initOnce == nil
fm.canSyncRunDecodeData = fm.canSyncRunMethods["DecodeData"] && fm.canSyncRunMethods["DecodeRequest"]
fm.canSyncRunDecodeTrailers = fm.canSyncRunMethods["DecodeTrailers"] && fm.canSyncRunMethods["DecodeRequest"]
fm.canSyncRunEncodeHeaders = fm.canSyncRunMethods["EncodeHeaders"]
fm.canSyncRunEncodeData = fm.canSyncRunMethods["EncodeData"] && fm.canSyncRunMethods["EncodeResponse"]
fm.canSyncRunEncodeTrailers = fm.canSyncRunMethods["EncodeTrailers"] && fm.canSyncRunMethods["EncodeResponse"]

return wrapFilterManager(fm)
}
Expand Down Expand Up @@ -465,27 +438,27 @@
filterWrappers[i] = model.NewFilterWrapper(name, f)
}

c.CanSkipMethodOnce.Do(func() {
canSkipMethod := newSkipMethodsMap()
canSyncRunMethod := newSyncRunMethodMap()
c.CanSkipMethodsOnce.Do(func() {
canSkipMethods := api.NewAllMethodsMap()
canSyncRunMethods := api.NewAllMethodsMap()
for _, fw := range filterWrappers {
f := fw.Filter
fc := c.FilterConfigs[fw.Name]
for meth := range canSkipMethod {
for meth := range canSkipMethods {
overridden, err := reflectx.IsMethodOverridden(f, meth)
if err != nil {
api.LogErrorf("failed to check method %s in filter: %v", meth, err)
// canSkipMethod[meth] will be false
// canSkipMethods[meth] will be false

Check warning on line 451 in api/pkg/filtermanager/filtermanager.go

View check run for this annotation

Codecov / codecov/patch

api/pkg/filtermanager/filtermanager.go#L451 

Added line #L451 was not covered by tests
}
canSkipMethod[meth] = canSkipMethod[meth] && !overridden
canSkipMethods[meth] = canSkipMethods[meth] && !overridden

if overridden {
canSyncRunMethod[meth] = canSyncRunMethod[meth] && fc.SyncRunPhases.Contains(api.MethodToPhase(meth))
canSyncRunMethods[meth] = canSyncRunMethods[meth] && fc.SyncRunPhases.Contains(api.MethodToPhase(meth))
}
}
}
c.CanSkipMethod = canSkipMethod
c.CanSyncRunMethod = canSyncRunMethod
c.CanSkipMethods = canSkipMethods
c.CanSyncRunMethod = canSyncRunMethods
})

if needLogExecution() {
Expand All @@ -502,21 +475,21 @@
}
}

canSkipMethod := c.CanSkipMethod
m.canSkipDecodeData = m.canSkipDecodeData && canSkipMethod["DecodeData"] && canSkipMethod["DecodeRequest"]
m.canSkipDecodeTrailers = m.canSkipDecodeTrailers && canSkipMethod["DecodeTrailers"] && canSkipMethod["DecodeRequest"]
m.canSkipEncodeHeaders = m.canSkipEncodeData && canSkipMethod["EncodeHeaders"]
m.canSkipEncodeData = m.canSkipEncodeData && canSkipMethod["EncodeData"] && canSkipMethod["EncodeResponse"]
m.canSkipEncodeTrailers = m.canSkipEncodeTrailers && canSkipMethod["EncodeTrailers"] && canSkipMethod["EncodeResponse"]
m.canSkipOnLog = m.canSkipOnLog && canSkipMethod["OnLog"]
canSkipMethods := c.CanSkipMethods
m.canSkipDecodeData = m.canSkipDecodeData && canSkipMethods["DecodeData"] && canSkipMethods["DecodeRequest"]
m.canSkipDecodeTrailers = m.canSkipDecodeTrailers && canSkipMethods["DecodeTrailers"] && canSkipMethods["DecodeRequest"]
m.canSkipEncodeHeaders = m.canSkipEncodeData && canSkipMethods["EncodeHeaders"]
m.canSkipEncodeData = m.canSkipEncodeData && canSkipMethods["EncodeData"] && canSkipMethods["EncodeResponse"]
m.canSkipEncodeTrailers = m.canSkipEncodeTrailers && canSkipMethods["EncodeTrailers"] && canSkipMethods["EncodeResponse"]
m.canSkipOnLog = m.canSkipOnLog && canSkipMethods["OnLog"]

// Similar to the skip check
canSyncRunMethod := c.CanSyncRunMethod
m.canSyncRunDecodeData = m.canSyncRunDecodeData && canSyncRunMethod["DecodeData"] && canSyncRunMethod["DecodeRequest"]
m.canSyncRunDecodeTrailers = m.canSyncRunDecodeTrailers && canSyncRunMethod["DecodeTrailers"] && canSyncRunMethod["DecodeRequest"]
m.canSyncRunEncodeHeaders = m.canSyncRunEncodeData && canSyncRunMethod["EncodeHeaders"]
m.canSyncRunEncodeData = m.canSyncRunEncodeData && canSyncRunMethod["EncodeData"] && canSyncRunMethod["EncodeResponse"]
m.canSyncRunEncodeTrailers = m.canSyncRunEncodeTrailers && canSyncRunMethod["EncodeTrailers"] && canSyncRunMethod["EncodeResponse"]
canSyncRunMethods := c.CanSyncRunMethod
m.canSyncRunDecodeData = m.canSyncRunDecodeData && canSyncRunMethods["DecodeData"] && canSyncRunMethods["DecodeRequest"]
m.canSyncRunDecodeTrailers = m.canSyncRunDecodeTrailers && canSyncRunMethods["DecodeTrailers"] && canSyncRunMethods["DecodeRequest"]
m.canSyncRunEncodeHeaders = m.canSyncRunEncodeData && canSyncRunMethods["EncodeHeaders"]
m.canSyncRunEncodeData = m.canSyncRunEncodeData && canSyncRunMethods["EncodeData"] && canSyncRunMethods["EncodeResponse"]
m.canSyncRunEncodeTrailers = m.canSyncRunEncodeTrailers && canSyncRunMethods["EncodeTrailers"] && canSyncRunMethods["EncodeResponse"]

// TODO: add field to control if merging is allowed
i := 0
Expand Down
Loading