-
Notifications
You must be signed in to change notification settings - Fork 419
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
contrib/k8s.io/client-go: inject the traceID as kube auditID #351
Conversation
06636c6
to
2e4b22e
Compare
} | ||
kubeAuditID := strconv.FormatUint(traceID, 10) | ||
req.Header.Set("Audit-Id", kubeAuditID) | ||
span.SetTag("kubernetes.auditID", kubeAuditID) |
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 think kubernetes.audit_id
would be more standard to APM (and the actual header itself). Would this change be troublesome for you?
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.
Not at all 👍
})) | ||
} | ||
|
||
func requestToResource(method, path string) string { | ||
func RequestToResource(method, path string) string { |
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.
Why was this made public API? If there's a good reason it's fine, but then let's clearly document it.
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 would like to use this part of the code to generate traces from the apiserver side while parsing the Kubernetes audit logs.
It avoids me to repeat this function and keep a coherence there.
The goal of this is to set a distributed trace.
Does it make sense ?
I'll document this properly.
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.
Sure. Let's keep it then. Was just making sure this was intentional.
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.
Looks good, happy to merge this once the small changes are applied.
})) | ||
} | ||
|
||
func requestToResource(method, path string) string { | ||
func RequestToResource(method, path string) string { |
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.
Sure. Let's keep it then. Was just making sure this was intentional.
Co-Authored-By: JulienBalestra <julien.balestra@gmail.com>
Co-Authored-By: JulienBalestra <julien.balestra@gmail.com>
1f0ddbe
to
3271bf4
Compare
traceID := span.Context().TraceID() | ||
if traceID == 0 { | ||
// tracer is not running | ||
return |
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.
What's wrong with the alignment here?
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 accepted the github suggestion 😆 going to fix it in the old way
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.
Ok then I'll take the fall for that 😅
Signed-off-by: JulienBalestra <julien.balestra@datadoghq.com>
8951903
to
4a9ba3a
Compare
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.
Thanks for this change and for being patient with my review.
Will include this in 1.5.0 next week. |
This allows to provide the Kubernetes Audit-ID to the kube-apiserver see.
With this change, we could then use datadog distributing tracing capabilities (audit logs) in the apiserver side.