-
Notifications
You must be signed in to change notification settings - Fork 40k
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
introduce a proper trace context #127551
introduce a proper trace context #127551
Conversation
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
LGTM label has been added. Git tree hash: 26bc78f9feb18c0bc438138d282e82462dbfb162
|
/assign @apelisse |
/sig instrumentation |
/assign @sttts |
@@ -171,6 +171,7 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope *RequestScope, forceWatc | |||
ctx := req.Context() | |||
// For performance tracking purposes. | |||
ctx, span := tracing.Start(ctx, "List", traceFields(req)...) | |||
req = req.WithContext(ctx) |
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.
we don't have this for any other verb. Is it only needed for watch? From the PR description it does not look like. Why don't we add that everywhere to get proper serialization spans?
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.
@sttts sorry, I forgot to add it for other verbs. updated.
Signed-off-by: carlory <baofa.fan@daocloud.io> Co-authored-by: CasperLiu <qiuyuzhe521@gmail.com>
2b99e93
to
5b2632f
Compare
@@ -98,6 +98,7 @@ func SerializeObject(mediaType string, encoder runtime.Encoder, hw http.Response | |||
attribute.String("protocol", req.Proto), | |||
attribute.String("mediaType", mediaType), | |||
attribute.String("encoder", string(encoder.Identifier()))) | |||
req = req.WithContext(ctx) |
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.
this one is not needed, is it? req
is not used anywhere below.
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.
it is used in
kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go
Line 106 in 99ff62e
contentEncoding: negotiateContentEncoding(req), |
@@ -55,6 +55,7 @@ func createHandler(r rest.NamedCreater, scope *RequestScope, admit admission.Int | |||
ctx := req.Context() | |||
// For performance tracking purposes. | |||
ctx, span := tracing.Start(ctx, "Create", traceFields(req)...) | |||
req = req.WithContext(ctx) |
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.
@dashpole these changes look good?
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.
nvmd. @dashpole lgmt'ed the change to the list handler before.
/lgtm |
LGTM label has been added. Git tree hash: d4334e9155a0c56dd7186189a09aff75b4305693
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carlory, dashpole, sttts 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
In Kubernetes release 1.28, the tracing structure for the API server is depicted as follows:
Within the API server's code structure, the spans "List(recursive=true) etcd3" and "SerializeObject" are expected to be nested under the "List" span. However, "SerializeObject" appears parallel to "List", which is not the intended behavior.
staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go
Which issue(s) this PR fixes:
Fixes #124073
Special notes for your reviewer:
Thank @CasperLiu but your PR #124079 is inactive and will be closed by rebot. so I am taking over it and adding you as a co-author. now, it has been changed as @jpbetz suggested.
If you want to continue your PR, please let me know, and feel free to close my PR. Thanks again.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: