Allow to set the decisionID from an external function #516
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pushpalanka
force-pushed
the
external-decision-id
branch
from
bfa0bda to
67beeaf
Compare
ashutosh-narkar
force-pushed
the
external-decision-id
branch
from
67beeaf to
35a961a
Compare
There was a problem hiding this comment.
Thanks for working on this @Pushpalanka! The changes lgtm. Just few comments below and we can then get this in.
envoyauth/response.go
Outdated
| @@ -34,14 +34,24 @@ type StopFunc = func() | |||
| // TransactionCloser should be called to abort the transaction | |||
| type TransactionCloser func(ctx context.Context, err error) error | |||
|
|
|||
| // An Opt allowing externally setting the decision ID | |||
| type Opt func(*EvalResult) | |||
There was a problem hiding this comment.
Nit: do we need to define this?
There was a problem hiding this comment.
We can remove it. Thanks for the suggestion.
envoyauth/response.go
Outdated
| // NewEvalResult creates a new EvalResult and a StopFunc that is used to stop the timer for metrics | ||
| func NewEvalResult() (*EvalResult, StopFunc, error) { | ||
| func NewEvalResult(opts ...Opt) (*EvalResult, StopFunc, error) { |
There was a problem hiding this comment.
Can we do something like:
func NewEvalResult(opts ...func(*EvalResult)) (*EvalResult, StopFunc, error)
Pushpalanka
force-pushed
the
external-decision-id
branch
from
7e0fc92 to
3635f3f
Compare
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Pushpalanka
force-pushed
the
external-decision-id
branch
from
3635f3f to
b01d47f
Compare
Thanks @ashutosh-narkar for your support as well. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Short description
An improvement to allow sending the decision ID from outside avoiding the default UUID generation here
Expected behavior
Suggesting improvement will allow passing a function from outside which can operate on the result. Current behavior of the default UUID generation is left intact, unless the function set a decision ID.
Additional context
In a benchmark test we noticed the UUID generation is comparatively costly. (Below image shows a branch of it.)
Hence it will benefit if we can send an externally generated ID and set it here, avoiding the costly operation.
CC: @mjungsbluth