Skip to content

Configure OpenTelemetry Tracing #4883

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 25, 2021
Merged

Configure OpenTelemetry Tracing #4883

merged 3 commits into from
Oct 25, 2021

Conversation

sallyom
Copy link
Contributor

@sallyom sallyom commented May 10, 2021 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR configures opentelemetry tracer provider and exporter and adds initial spans.
This PR follows the example of etcd & kube-apiserver
etcd: etcd-io/etcd#12919
kube-apiserver: kubernetes/kubernetes#94942

Which issue(s) this PR fixes:

#4734

Does this PR introduce a user-facing change?

The option to export OpenTelemetry trace data has been added. This is experimental and not enabled by default. To enable tracing, configure the tracing section of the crio configuration file.

@sallyom sallyom requested review from mrunalp and runcom as code owners May 10, 2021 14:58
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 10, 2021
@openshift-ci openshift-ci bot requested a review from vrothberg May 10, 2021 14:58
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 10, 2021

Hi @sallyom. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 10, 2021
haircommander
haircommander reviewed May 10, 2021
View reviewed changes
@sallyom sallyom force-pushed the otel-trace-2 branch 4 times, most recently from 14900e1 to 19373b3 Compare May 11, 2021 04:47
@sallyom sallyom mentioned this pull request May 11, 2021
Closed
@sallyom
Copy link
Contributor Author

sallyom commented May 11, 2021

/cc @fidencio Is the replacement for grpc version still required? I hope not!
#4843 (comment)

@sallyom sallyom changed the title WIP: Configure OpenTelemetry Traces Configure OpenTelemetry Tracing May 11, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2021
@haircommander
Copy link
Member

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 11, 2021
@fidencio
Copy link
Contributor

/cc @fidencio Is the replacement for grpc version still required? I hope not!
#4843 (comment)

Hmm. I think it still is, I'm afraid.

@fidencio
Copy link
Contributor

/cc @fidencio Is the replacement for grpc version still required? I hope not!
#4843 (comment)

Hmm. I think it still is, I'm afraid.

/cc @fgiudici, would you have time to check this out? What's the status of your PR on containerd side?

@fgiudici
Copy link
Contributor

/cc @fidencio Is the replacement for grpc version still required? I hope not!
#4843 (comment)

Hmm. I think it still is, I'm afraid.

/cc @fgiudici, would you have time to check this out? What's the status of your PR on containerd side?

The patch (containerd/ttrpc#67 ) hasn't been merged as containerd folks prefer to switch the gogo protobuf to the google protobuf. That would be the future proof fix (as gogo protobuf is no more supported).
Not clear anyway when and who will do this, as time is flowing away and seems the issue is just ignored.

@fidencio
Copy link
Contributor

/cc @fidencio Is the replacement for grpc version still required? I hope not!
#4843 (comment)

Hmm. I think it still is, I'm afraid.

I just tested this PR and I see absolutely no issue on starting a pod using kata-containers.
I think we may be good to go? @fgiudici, if you want to double check it, just to be sure I'm not missing something ...

@fgiudici
Copy link
Contributor

/cc @fidencio Is the replacement for grpc version still required? I hope not!
#4843 (comment)

Hmm. I think it still is, I'm afraid.

I just tested this PR and I see absolutely no issue on starting a pod using kata-containers.
I think we may be good to go? @fgiudici, if you want to double check it, just to be sure I'm not missing something ...

I have to take some time to look at the code to recall how exactly the issue happened. I can do that tomorrow.

@sallyom
Copy link
Contributor Author

sallyom commented May 11, 2021

/retest

@fidencio
Copy link
Contributor

kata-containers CI is failing due to:

21:33:29 # go.opentelemetry.io/otel/trace
21:34:02 ../../../../pkg/mod/go.opentelemetry.io/otel/[email protected]/config.go:117:2: duplicate method private
21:34:02 note: module requires Go 1.14
21:34:02 Makefile:165: recipe for target 'bin/crio' failed
21:34:47 make: *** [bin/crio] Error 2
21:34:47 Failed at 92: make BUILDTAGS='exclude_graphdriver_btrfs exclude_graphdriver_devicemapper libdm_no_deferred_remove'
21:34:47 CRI-O not installed
21:34:47 containerd not installed

@sallyom sallyom force-pushed the otel-trace-2 branch 2 times, most recently from fd01f31 to 1baa3ad Compare October 20, 2021 16:00
@sallyom
Copy link
Contributor Author

sallyom commented Oct 20, 2021

/test critest_fedora
/test e2e_crun
/test e2e_rhel

sallyom
sallyom commented Oct 20, 2021
View reviewed changes
@sallyom
Copy link
Contributor Author

sallyom commented Oct 20, 2021

@haircommander ptal and thank you!

@sallyom
Copy link
Contributor Author

sallyom commented Oct 21, 2021

/retest-required

@sallyom
Copy link
Contributor Author

sallyom commented Oct 21, 2021 •
edited
Loading

@haircommander @saschagrunert all tests are green, also, I added [EXPERIMENTAL] tags, updated docs/man ptal!!

@haircommander
Copy link
Member

the PR generally looks good to me.

I would love if we could have some tests showing how this all works e2e in cri-o. The integration suite in test/ directory is a good place I think, though, without support in crictl, is that even possible?

If not, I think it would also be helpful to have some documentation about how to configure everything e2e. That could live in the tutorials/ directory.

Both/either/neither can be done as follow ups though

/approve

Thanks for all your hard work 🙂

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2021
@sallyom
Copy link
Contributor Author

sallyom commented Oct 21, 2021

@haircommander, I do have tutorials/docs/video of all this working, I will definitely have a follow-up PR coming w/in the next few days to show how to enable & collect.

saschagrunert
saschagrunert approved these changes Oct 25, 2021
View reviewed changes
sallyom and others added 3 commits October 25, 2021 08:26
@sallyom @husky-parul
opentelemetry: add gRPC tracing
c16429e 
Signed-off-by: Sally O'Malley <[email protected]>

Co-authored-by: husky-parul <[email protected]>
@sallyom
pin go.opentelemetry grpc/otelgrpc v0.25.0
37418e1 
Signed-off-by: Sally O'Malley <[email protected]>
@sallyom
bump(deps-opentelemetry)
ec63057 
Signed-off-by: Sally O'Malley <[email protected]>
saschagrunert
saschagrunert approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, sallyom, saschagrunert

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:
  • OWNERS [haircommander,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@saschagrunert
Copy link
Member

/retest-required

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2021 •
edited
Loading

@sallyom: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/openshift-jenkins/e2e_crun_cgroupv2 e4a7a7d link false /test e2e_cgroupv2
ci/openshift-jenkins/integration_crun_cgroupv2 ec63057 link false /test integration_cgroupv2

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@sallyom
Copy link
Contributor Author

sallyom commented Oct 25, 2021

/retest-required

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit bf8bc04 into cri-o:main Oct 25, 2021
@sallyom sallyom mentioned this pull request Oct 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haircommander haircommander haircommander left review comments

@husky-parul husky-parul husky-parul left review comments

@sonatype-lift sonatype-lift[bot] sonatype-lift[bot] left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@runcom runcom Awaiting requested review from runcom

@vrothberg vrothberg Awaiting requested review from vrothberg

Assignees

@saschagrunert saschagrunert

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@sallyom @haircommander @fidencio @fgiudici @husky-parul @littlejawa @saschagrunert @openshift-bot @openshift-merge-robot