feat: improve tracing

[alnr]

needs ory/x#650

[hperl]

LGTM 🎉

[aeneasr]

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

[alnr]

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.

[aeneasr]

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())}
	}

[alnr]

in the top branch, if result.Err is not nil, we would miss that error. Not sure if that can ever happen 🤷

[alnr]

@aeneasr do you still want me to make this change? Otherwise this PR is ready to merge.

[aeneasr]

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 :)

[CaptainStandby]

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.
See also:

• https://stackoverflow.com/a/2391555
• https://en.wikipedia.org/wiki/Copyright_notice

We probably should fix the automatic tooling

[alnr]

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. See also:

• https://stackoverflow.com/a/2391555
• https://en.wikipedia.org/wiki/Copyright_notice

We probably should fix the automatic tooling

I did it on purpose (with make format) because the format job failed in CI. You are probably correct, though.

[CaptainStandby]

I did it on purpose (with make format) because the format job failed in CI. You are probably correct, though.

I have created a PR for fixing this here