Skip to content

Add integration test to opentelemetry tracing on image pull#7847

Merged
samuelkarp merged 1 commit intocontainerd:mainfrom
fangn2:adding-integration-test-to-opentelemetry
Jan 31, 2023
Merged

Add integration test to opentelemetry tracing on image pull#7847
samuelkarp merged 1 commit intocontainerd:mainfrom
fangn2:adding-integration-test-to-opentelemetry

Conversation

@fangn2
Copy link
Copy Markdown
Contributor

@fangn2 fangn2 commented Dec 20, 2022 •
edited
Loading

This PR is to address #7493

This PR adds
Integration test to tracing at client side on image pull following steps:

  • Create an in-memory exporter and global tracer provider
  • Pull image with client which should create spans
  • Check if there are spans in the exporter

Those steps can be used for other tracing related integration tests too.

Signed-off-by: Tony Fang [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Dec 20, 2022

Hi @fangn2. Thanks for your PR.

I'm waiting for a containerd 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.

Details

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.

@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from c94456b to 35da547 Compare December 20, 2022 20:42
@fangn2 fangn2 marked this pull request as draft December 21, 2022 14:48
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from 35da547 to 88bc2e5 Compare December 21, 2022 20:45
@fangn2 fangn2 changed the title Adding unit/integration test to opentelemetry tracing Add unit/integration test to opentelemetry tracing Dec 21, 2022
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch 4 times, most recently from a92a200 to 1935e8c Compare December 22, 2022 15:26
@fangn2 fangn2 marked this pull request as ready for review December 22, 2022 16:11
swagatbora90
swagatbora90 reviewed Dec 22, 2022
View reviewed changes
Comment thread integration/client/tracing_test.go Outdated
swagatbora90
swagatbora90 reviewed Dec 22, 2022
View reviewed changes
Comment thread integration/client/tracing_test.go Outdated
swagatbora90
swagatbora90 reviewed Dec 22, 2022
View reviewed changes
Comment thread integration/client/tracing_test.go Outdated
swagatbora90
swagatbora90 reviewed Dec 22, 2022
View reviewed changes
Comment thread integration/client/tracing_test.go Outdated
swagatbora90
swagatbora90 reviewed Dec 22, 2022
View reviewed changes
Comment thread plugin/plugin_test.go
@fangn2 fangn2 marked this pull request as draft December 28, 2022 21:28
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch 3 times, most recently from cbb364f to 99f40cc Compare January 3, 2023 16:11
@fangn2
Copy link
Copy Markdown
Contributor Author

fangn2 commented Jan 3, 2023

@swagatbora90 Thanks for your comments, updated in the latest commit.

@fangn2 fangn2 marked this pull request as ready for review January 3, 2023 16:52
@fangn2
Copy link
Copy Markdown
Contributor Author

fangn2 commented Jan 5, 2023 •
edited
Loading

CI failure on integration test not related to the change.

=== Failed
=== FAIL: . TestTransferEcho/ImportExportEchoBig (0.00s)
    transfer_test.go:50: rpc error: code = NotFound desc = not found
    --- FAIL: TestTransferEcho/ImportExportEchoBig (0.00s)

=== FAIL: . TestTransferEcho (0.01s)
    log_hook.go:47: time="[20](https://github.com/containerd/containerd/actions/runs/3831076914/jobs/6519953513#step:15:21)23-01-03T16:38:51.[21](https://github.com/containerd/containerd/actions/runs/3831076914/jobs/6519953513#step:15:22)4279679Z" level=error msg="error copying stream" func="archive.(*ImageExportStream).MarshalAny.func1" file="/home/runner/work/containerd/containerd/pkg/transfer/archive/exporter.go:65" error="received failed: context canceled" streamid=export-207317833-SpnO

DONE 153 tests, 2 failures in [22](https://github.com/containerd/containerd/actions/runs/3831076914/jobs/6519953513#step:15:23)1.9[26](https://github.com/containerd/containerd/actions/runs/3831076914/jobs/6519953513#step:15:27)s
make: *** [Makefile:215: integration] Error 1
Error: Process completed with exit code 2.

Same issue as described in #7884
Rebased the main branch to solve it.
Now failing on Windows integration tests which are not related to the change.

@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from 99f40cc to 675e8e5 Compare January 5, 2023 19:25
@fangn2 fangn2 changed the title Add unit/integration test to opentelemetry tracing Add integration test to opentelemetry tracing on image pull Jan 5, 2023
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from 675e8e5 to c3b37a9 Compare January 9, 2023 14:21
@fangn2
Copy link
Copy Markdown
Contributor Author

fangn2 commented Jan 9, 2023

Rebased to fix Windows CI issue.

swagatbora90
swagatbora90 reviewed Jan 9, 2023
View reviewed changes
Comment thread integration/client/client_test.go Outdated
swagatbora90
swagatbora90 reviewed Jan 9, 2023
View reviewed changes
Comment thread integration/client/client_test.go Outdated
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from c3b37a9 to d634ee0 Compare January 10, 2023 04:29
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from d634ee0 to 2dede6f Compare January 10, 2023 04:50
@fangn2 fangn2 marked this pull request as draft January 10, 2023 16:39
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from 2dede6f to 6f6f1f1 Compare January 10, 2023 21:50
@fangn2 fangn2 marked this pull request as ready for review January 10, 2023 22:54
@fangn2
Copy link
Copy Markdown
Contributor Author

fangn2 commented Jan 10, 2023

Pushed changes to address comments.

CI failing on installing dependencies not related to the change.

Fetched 402 kB in 0s (3341 kB/s)
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/g/gperf/gperf_3.1-1build1_amd64.deb  503  Service Unavailable [IP: 52.154.174.208 80]
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
Error: Process completed with exit code 100.

estesp
estesp approved these changes Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from 6f6f1f1 to 4bd31bc Compare January 10, 2023 23:30
swagatbora90
swagatbora90 reviewed Jan 10, 2023
View reviewed changes
Comment thread integration/client/tracing.go Outdated
swagatbora90
swagatbora90 approved these changes Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@swagatbora90 swagatbora90 left a comment

Choose a reason for hiding this comment

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

One nit comment otherwise LGTM!

@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch 3 times, most recently from 179c437 to b186b0c Compare January 13, 2023 19:51
kzys
kzys reviewed Jan 13, 2023
View reviewed changes
Comment thread integration/client/tracing.go Outdated
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from b186b0c to 2a165a1 Compare January 13, 2023 20:28
@fangn2
Copy link
Copy Markdown
Contributor Author

fangn2 commented Jan 13, 2023

Updated to address comments(move tracer provider global setting to caller side)

CI is failing not related to my change.

=== Failed
=== FAIL: . TestContentClient/Resume (0.70s)
    testsuite.go:278: ref ContentClient-n1/1/cb locked for 288.5072ms (since 2023-01-13 20:54:24.5045207 +0000 GMT m=+7.586877701): unavailable
    helpers.go:66: drwxrwxrwx          0 C:\Users\RUNNER~1\AppData\Local\Temp\content-suite-ContentClient-2088116406
    --- FAIL: TestContentClient/Resume (0.70s)

=== FAIL: . TestContentClient (12.52s)

@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from 2a165a1 to 3654e39 Compare January 24, 2023 18:42
@fangn2 fangn2 requested a review from kzys January 24, 2023 18:43
@kzys
Copy link
Copy Markdown
Member

kzys commented Jan 26, 2023

/ok-to-test

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Jan 31, 2023

Looks good but needs rebase for go mod

@fangn2
Add integration test for tracing on image pull
c46aaa8 
Create an in-memory exporter and global tracer provider
Pull image with client which should create spans
Validate spans in the exporter

Signed-off-by: Tony Fang <[email protected]>
@fangn2 fangn2 force-pushed the adding-integration-test-to-opentelemetry branch from 3654e39 to c46aaa8 Compare January 31, 2023 05:46
@fangn2
Copy link
Copy Markdown
Contributor Author

fangn2 commented Jan 31, 2023

Thanks for checking @dmcgowan.
Rebased to fix go mod conflict.

@samuelkarp samuelkarp merged commit e307f87 into containerd:main Jan 31, 2023
@fangn2 fangn2 mentioned this pull request Jan 31, 2023
Closed
@fangn2 fangn2 deleted the adding-integration-test-to-opentelemetry branch February 8, 2023 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kzys kzys kzys approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

@samuelkarp samuelkarp samuelkarp approved these changes

@estesp estesp estesp approved these changes

+1 more reviewer

@swagatbora90 swagatbora90 swagatbora90 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ok-to-test

Projects

Pull Request Review
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fangn2 @k8s-ci-robot @kzys @dmcgowan @samuelkarp @estesp @swagatbora90