Add tracing to apiserver client-go requests #103218
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
/sig instrumentation |
k8s-ci-robot
added
priority/important-soon
|
/test all |
|
/test all |
|
/retest |
dashpole
changed the title
|
/retest |
| if err := o.EgressSelector.ApplyTo(&config.Config); err != nil { | ||
| return err | ||
| } | ||
| if feature.DefaultFeatureGate.Enabled(features.APIServerTracing) { | ||
| // Apply tracing config right after CoreAPI and EgressSelector so that the rest config is | ||
| // updated with a TracerProvider before anyone tries to use it. | ||
| if wrapper, err := o.Traces.ApplyTo(config.Config.EgressSelector, &config.Config); err != nil { | ||
| return err | ||
| } else if config.TracerProvider != nil { | ||
| config.ClientConfig.Wrap(wrapper) | ||
| } | ||
| } |
There was a problem hiding this comment.
is the ordering significant or did you just group these for readability?
There was a problem hiding this comment.
The tracing ApplyTo function relies on the EgressSelector already being configured, and should come immediately after the CoreAPI ApplyTo so that any users of the client use our wrapper, and get traces setup.
There was a problem hiding this comment.
clientgoExternalClient, err := clientgoclientset.NewForConfig(kubeconfig) is constructed/used inside CoreAPIOptions#ApplyTo ... that won't have tracing
There was a problem hiding this comment.
I moved wrapping the client to inside CoreAPI. Conveniently, it now captures both the external client, and the one embedded in config.
k8s-ci-robot
added
the
lgtm
|
/assign @liggitt |
cmd/kube-apiserver/app/server.go
Outdated
| @@ -382,7 +382,7 @@ func buildGenericConfig( | |||
| return | |||
| } | |||
| if utilfeature.DefaultFeatureGate.Enabled(genericfeatures.APIServerTracing) { | |||
| if lastErr = s.Traces.ApplyTo(genericConfig.EgressSelector, genericConfig); lastErr != nil { | |||
| if _, lastErr = s.Traces.ApplyTo(genericConfig.EgressSelector, genericConfig); lastErr != nil { | |||
| return | |||
| } | |||
There was a problem hiding this comment.
should this be setting genericConfig.LoopbackClientConfig.Wrap below?
There was a problem hiding this comment.
I am wrapping LoopbackClientConfig within Traces.ApplyTo, since it is common between staging/src/k8s.io/apiserver/pkg/server/options/recommended.go and cmd/kube-apiserver/app/server.go. I could instead move it out if there is a place it would make more sense in.
k8s-ci-robot
removed
the
lgtm
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, liggitt, logicalhan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This adds tracing for client-go requests, which will help debug reentrant api calls, even if they are not part of the same trace (#103186).
Part of enhancement: kubernetes/enhancements#647
Special notes for your reviewer:
In order to avoid globals (as requested in the initial tracing review), we pass the TracerProvider via config instead of using the global one.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: