-
Notifications
You must be signed in to change notification settings - Fork 460
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
Add support for OTEL tracing #1319
Conversation
0545993
to
db27260
Compare
491823a
to
425a995
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.
/lgtm
/hold
hold for other maintainers to look at.
Everything looks good, thank you for adding the hack/tracing
. I think what can be useful as an additional improvements:
- Add a start span/stop span operations on individual crictl commands so commands will group all the calls they are making and we will also see failed commands - when they failed with arguments validation.
- I wonder if we may need to hide any sensitive information off the arguments passed. Since it is an optional feature, as-is OK, but maybe later we can add an option to filter out sensitive information
- We may want to add a computer name as part of a span so we can group spans by it. This way we can see from jaeger who called crictl command
Thanks! I'll follow-up on those 👍
We have included the host name: https://github.com/kubernetes-sigs/cri-tools/pull/1319/files#diff-f1e0d24911164f850e8c1e92b5e73073d417eb5ca067ad3feba01bc6d6211082R34-R43 Or do you have something more specific in mind? |
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. Thanks adding this!
@saschagrunert could you rebase? |
425a995
to
7d06f5b
Compare
66bb77f
to
29f9113
Compare
@feiskyer done :) |
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
29f9113
to
9a77979
Compare
9a77979
to
21a528f
Compare
Addressed the comments, thank you for the review @kwilczynski @SergeyKanzhelev. PTAL again. |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, saschagrunert, SergeyKanzhelev 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 |
@SergeyKanzhelev can we lift the hold? |
`crictl` now features 3 new CLI parameters: - `--enable-tracing`: Enable OpenTelemetry tracing. (default: false) - `--tracing-endpoint`: Address to which the gRPC tracing collector will send spans to. (default: "0.0.0.0:4317") - `--tracing-sampling-rate-per-million`: Number of samples to collect per million OpenTelemetry spans. Set to 1000000 or -1 to always sample. (default: -1) The tracer provider will be created on startup and the `Shutdown()` invocation will ensure that all spans are processed before exiting the binary. The `hack/tracing` directory contains scripts for local testing: ``` > ./hack/tracing/start … Everything is ready, open http://localhost:16686 to access jaeger ``` When now running `crictl` with `--enable-tracing`: ``` > sudo ./build/bin/linux/amd64/crictl --enable-tracing ps ``` Then jaeger should show collected traces and spans for the 3 RPCs `ListContainers`, `ImageFsInfo` as well as `Version`. Signed-off-by: Sascha Grunert <[email protected]>
21a528f
to
f6e57a2
Compare
/unhold sorry for delay /lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
crictl
now features 3 new CLI parameters:--enable-tracing
: Enable OpenTelemetry tracing. (default: false)--tracing-endpoint
: Address to which the gRPC tracing collector will send spans to. (default: "0.0.0.0:4317")--tracing-sampling-rate-per-million
: Number of samples to collect per million OpenTelemetry spans. Set to 1000000 or -1 to always sample. (default: -1)The tracer provider will be created on startup and the
Shutdown()
invocation will ensure that all spans are processed before exiting the binary.The
hack/tracing
directory contains scripts for local testing:When now running
crictl
with--enable-tracing
:Then jaeger should show collected traces and spans for the 3 RPCs
ListContainers
,ImageFsInfo
as well asVersion
:Which issue(s) this PR fixes:
Fixes #1317
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?