-
-
Notifications
You must be signed in to change notification settings - Fork 347
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.
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
feat: improve tracing #1169
feat: improve tracing #1169
Conversation
09b53e1
to
f6ed553
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
defer func() { | ||
if res.Err != nil { | ||
span.SetStatus(codes.Error, res.Err.Error()) | ||
} else { | ||
span.SetAttributes(attribute.String("membership", res.Membership.String())) | ||
} | ||
span.End() | ||
}() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the function has only two returns, why do we need a defer for this? You could also do:
span.SetAttributes(attribute.String("membership", res.Membership.String()))
return result
err := ...
span.SetStatus(codes.Error, res.Err.Error())
return err
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two possible error returns from the function:
<-ctx.Done()
<-resultCh
returns a struct with the Err field set.
Checking the named return value in the deferred call catches them both. Without that, the branch (if res.Err != nil
) would need to move inside the select
which is awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand - would this not just be:
resultCh := make(chan checkgroup.Result)
go e.checkIsAllowed(ctx, r, restDepth)(ctx, resultCh)
select {
case result := <-resultCh:
+ span.SetAttributes(attribute.String("membership", res.Membership.String()))
return result
case <-ctx.Done():
+ span.SetStatus(codes.Error, ctx.Err())
return checkgroup.Result{Err: errors.WithStack(ctx.Err())}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the top branch, if result.Err
is not nil
, we would miss that error. Not sure if that can ever happen 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aeneasr do you still want me to make this change? Otherwise this PR is ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a bit clonky to read compared to the two-liner, but it does guard against future regressions. It's also just a style choice so I'm fine either way - you decide :)
You changed all the license headers (probably automatically). I think this is wrong, as the year of these copyright markings normally reflect the year of first publication. We probably should fix the automatic tooling |
I did it on purpose (with |
I have created a PR for fixing this here |
needs ory/x#650