-
Notifications
You must be signed in to change notification settings - Fork 474
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
send custom headers in tracing backend requests #7353
send custom headers in tracing backend requests #7353
Conversation
297efcc
to
df3e436
Compare
I think the only thing left to do is figure out how to get custom headers sent via the gRPC client that is created here: https://github.com/kiali/kiali/blob/v1.84.0/tracing/client.go#L150-L158 conn, err := grpc.Dial(address, opts...)
if err == nil {
cc := model.NewQueryServiceClient(conn)
client, err = jaeger.NewGRPCJaegerClient(cc)
if err != nil {
return nil, err
}
log.Infof("Create %s GRPC client %s", cfgTracing.Provider, address)
return &Client{httpTracingClient: httpTracingClient, grpcClient: client, ctx: ctx}, nil
... Need to investigate the APIs used here because it isn't clear to me if you can set client-side headers to be sent over the underlying HTTP/2 transport. (UPDATE: latest commits do this. So this is done). |
a1e5ac1
to
c8f07a0
Compare
external_services:
tracing:
url: http://tracing.istio-system:16685/jaeger
custom_headers:
X-MazzWasHere_Foo: mazz_WasHere
|
1d99c61
to
358a565
Compare
This is all ready. I was able to confirm the header is passed with using gRPC and non-gRPC. |
@josunect I added you as a reviewer to take a look at the code since you are most familiar with it. The person who asked for this feature tested it in his cluster and it works for him, and I tested it (as you see above) and it works for me, too. I just want to make sure the code doesn't break something else that I cannot see. |
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 didn't run it because the community originator of the RFE already tested it successfully. Code looks good for me.
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 haven't tested it yet, I've just added one comment after looking at the code. Except for that, it looks good!
@@ -165,7 +171,7 @@ func newClient(ctx context.Context, cfg *config.Config, token string) (*Client, | |||
// Legacy HTTP client | |||
log.Tracef("Using legacy HTTP client for Tracing: url=%v, auth.type=%s", u, auth.Type) | |||
timeout := time.Duration(config.Get().ExternalServices.Tracing.QueryTimeout) * time.Second | |||
transport, err := httputil.CreateTransport(&auth, &http.Transport{}, timeout, nil) | |||
transport, err := httputil.CreateTransport(&auth, &http.Transport{}, timeout, cfgTracing.CustomHeaders) | |||
if err != nil { | |||
return nil, 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.
I can't add the comment in the specific line, but I think the ctx should also need to be added in the Tempo grpc client? This would be in line 195:
clientConn, _ := grpc.Dial(grpcAddress, dialOps...)
I think that could be replaced with something like:
clientConn, _ := grpc.DialContext(ctx, grpcAddress, dialOps...)
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 made this change - see commit abb5e7a
The failure is the known flake unrelated to this PR |
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.
Re-approving, +1 to Josune's suggested change.
fixes: #7266