-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
chore: added tracing also for authz remote #1056
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1056 +/- ##
=======================================
Coverage 77.79% 77.79%
=======================================
Files 81 81
Lines 3967 3967
=======================================
Hits 3086 3086
Misses 600 600
Partials 281 281
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Hi @omerlh, thanks for the PR! We've recently done some work to move Oathkeeper's instrumentation from OpenTracing to OpenTelemetry (#1047, #1050). Your change uses the (obsolete Would you mind changing your PR to instead inject the tracer into the authorizer's constructor instead? Something like this: // NewAuthorizerRemote creates a new AuthorizerRemote.
func NewAuthorizerRemote(c configuration.Provider, d interface{ Tracer() trace.Tracer }) *AuthorizerRemote {
return &AuthorizerRemote{
c: c,
client: httpx.NewResilientClient(httpx.ResilientClientWithTracer(d.Tracer())).StandardClient(),
t: x.NewTemplate("remote"),
}
} And change the call sites to something like
for tests and
in Thanks for your contribution! |
c66b69e
to
a378a18
Compare
Yeah sure, just pushed a fix |
Thank you! Seems like you missed a spot. Also kindly run |
70a57e9
to
c65421c
Compare
Yes, sorry! Fixed all the issues I think. Waiting for the tests 🙏 |
Any pointer on how I can solve the testing error? I have no idea why it is happening |
In c, err := configuration.NewKoanfProvider(context.Background(), nil, logrusx.New("", ""))
require.NoError(t, err)
r := NewRegistry(c) instead of r := NewRegistryMemory() (two occurences) |
a1895da
to
6a68e82
Compare
Thanks! That fixed it :) |
6a68e82
to
7cb06f8
Compare
Related issue(s)
Basically follow #995 just for authorizers
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments