refact: avoid mem alloc with disabled decision log #632
Conversation
envoyauth/response.go
Outdated
| txn, err := store.NewTransaction(ctx, params) | ||
| if err != nil { | ||
| noopCloser := func(ctx context.Context, err error) error { |
There was a problem hiding this comment.
nit
| noopCloser := func(ctx context.Context, err error) error { | |
| noopCloser := func(context.Context, error) error { |
There was a problem hiding this comment.
Perhaps this function literal is simple enough that it won't leak to the heap, but you could try to rewrite it a package level function, e.g.
func noopCloser(ctx context.Context, err error) error {
return nil // no-op default
}
and then reference that instead. Measure and see if it makes any difference :)
There was a problem hiding this comment.
I've made this change. However, there are no significant changes from the benchmark perspective.
| @@ -21,15 +20,10 @@ func (e *internalError) Error() string { | |||
| } | |||
|
|
|||
| // LogDecision - Logs a decision log event | |||
| func LogDecision(ctx context.Context, manager *plugins.Manager, info *server.Info, result *envoyauth.EvalResult, err error) error { | |||
There was a problem hiding this comment.
Technically this is a breaking change. Someone could be using this function from elsewhere.
The "fix" would be to add another static function here and keep the old one as-is. But I think it's very unlikely that someone cares for this function, so let's not be too timid about it. 👍
There was a problem hiding this comment.
Maybe it's worth to move decisionlog package under the internal directory.
There was a problem hiding this comment.
We will technically be releasing a major version (v1.0.0) of plugin in the next weeks so I'm also not too worried about breaking change... before we release v1 of plugin I'll try to take a look at what code we want to support as importable in the future for long-term.
There was a problem hiding this comment.
Skipper is using the function. It would have been nice to have a note in the release note mentioning it's a breaking change. Still since it's 1.0.0 breaking changes were expected.
Signed-off-by: Anthony Regeda <[email protected]>
regeda
force-pushed
the
avoid-memalloc-with-disabled-decisionlog
branch
from
d68c4ca to
d07f08e
Compare
| @@ -21,15 +20,10 @@ func (e *internalError) Error() string { | |||
| } | |||
|
|
|||
| // LogDecision - Logs a decision log event | |||
| func LogDecision(ctx context.Context, manager *plugins.Manager, info *server.Info, result *envoyauth.EvalResult, err error) error { | |||
There was a problem hiding this comment.
We will technically be releasing a major version (v1.0.0) of plugin in the next weeks so I'm also not too worried about breaking change... before we release v1 of plugin I'll try to take a look at what code we want to support as importable in the future for long-term.
…#632) Signed-off-by: Anthony Regeda <[email protected]>
Address breaking change from open-policy-agent/opa-envoy-plugin#632 Signed-off-by: Pushpalanka Jayawardhana <[email protected]>
Benchmark results.
Before:
After: