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
Migrate apiserver from utils/trace to component-base/tracing #113172
Migrate apiserver from utils/trace to component-base/tracing #113172
Conversation
Ah, I didn't realize k8s.io/utils was the source of truth for that library. I'll have to make an update there first. Edit: utils update kubernetes/utils#264. |
@swagatbora90 if you wanna do a code review! |
/cc @logicalhan |
4f7edb9
to
e250c0f
Compare
This is now ready for review. |
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
This looks strictly better than it used to be. Thanks for making these changes!
staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go
Outdated
Show resolved
Hide resolved
d150e5e
to
63e7f83
Compare
63e7f83
to
de26b90
Compare
/approve |
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
/approve
[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 |
/triage accepted |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Part of #113170
Depends on #113183. See that PR for the proposed changes to utils/traces interfaces.
After this change, APIServerTracing traces will include spans from calls to tracing.Start(), and span events from calls to span.AddEvent(). Log-based "tracing" will continue to function exactly the same, with the exception a change to the wording of
Step()
calls that included an error.If the APIServerTracing feature is disabled, the OpenTelemetry portion of these calls are no-ops.
Special notes for your reviewer:
Naming differences between utils/trace and component-base/tracing:
utiltrace.New()
, andutiltrace.Nest()
are replaced bytracing.Start()
span
instead oftrace
, which is more accuratetrace.Step()
is replaced byspan.AddEvent()
trace.LogIfLong()
is replaced byspan.End()
utiltrace.Field
is replaced byattribute.KeyValue
This is intended to simply be a refactor, and shouldn't significantly change behavior when using the logging trace functionality. The only behavioral change is that
trace.Step
calls which specify an error have been split into a "succeeded" step without an error, and a "failed" step with an error. This is because err.Error() panics if err is nil.In cases where a utiltrace.Trace was stored on an object, or passed via function, i've replaced those with Context-based passing of the current active span.
The component-base/tracing library gracefully handles invoking functions on spans retrieved from context. It is no longer necessary to check for nil before calling AddEvent or LogIfLong.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig instrumentation
/kind feature
/priority important-soon