Skip to content
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

tracer: Add SkyWalking tracer #13060

Merged
merged 73 commits into from
Nov 4, 2020
Merged

tracer: Add SkyWalking tracer #13060

merged 73 commits into from
Nov 4, 2020

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Sep 11, 2020

Commit Message: Tracer: new tracer for SkyWalking tracing
Additional Description:
A new tracer to support SkyWalking Tracing. Check issue #12486 for more information.

This work is based on @dio 's initial code. It is a great job and is very helpful for me to start this work. Thanks.

  • Complete docker-compose sandbox to test SkyWalking tracer.

Risk Level: Medium
Testing: unit test & need more test
Docs Changes: None.
Release Notes: None.

wbpcode added 2 commits September 11, 2020 21:15
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-watchers: FYI only for changes made to (bazel/repository_locations\.bzl)|(api/bazel/repository_locations\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #13060 was opened by wbpcode.

see: more, trace.

@dio
Copy link
Member

dio commented Sep 11, 2020

cc. @yskopets

@dio dio marked this pull request as draft September 11, 2020 13:30
@htuch htuch assigned dio and lizan Sep 11, 2020
CODEOWNERS Outdated Show resolved Hide resolved
wbpcode added 7 commits September 13, 2020 01:11
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

quick api review

api/envoy/config/trace/v3/skywalking.proto Outdated Show resolved Hide resolved
api/envoy/config/trace/v3/skywalking.proto Outdated Show resolved Hide resolved
wbpcode added 2 commits September 18, 2020 13:29
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
@wbpcode
Copy link
Member Author

wbpcode commented Sep 22, 2020

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13060 (comment) was created by @wbpcode.

see: more, trace.

CODEOWNERS Outdated Show resolved Hide resolved
@dio dio changed the title [WIP] Tracer: new tracer for SkyWalking tracing tracer: Add SkyWalking tracer Sep 22, 2020
@dio
Copy link
Member

dio commented Sep 22, 2020

@wbpcode do you think this is ready to review? I'll try to create a test repo (docker-compose) for this tomorrow morning so we can have a sandbox for this.

@wbpcode
Copy link
Member Author

wbpcode commented Sep 22, 2020

@wbpcode do you think this is ready to review? I'll try to create a test repo (docker-compose) for this tomorrow morning so we can have a sandbox for this.

@dio

Yes. I think it is ready to review. I did some simple manual tests in our own environment. But we use the Envoy version that we maintain independently, which is not exactly the same as the community version. And our SkyWalking is also an internal version, which is also different from the community. 😢

This will be great if you complete this sandbox. 😄

wbpcode added 3 commits September 25, 2020 19:59
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
@dio
Copy link
Member

dio commented Sep 30, 2020

cc. @basvanbeek

Signed-off-by: wbpcode <comems@msn.com>
@wbpcode
Copy link
Member Author

wbpcode commented Nov 1, 2020

@lizan Updated again. 😄 If there's anything else that needs to be optimized, just ask and I'll fix it as soon as possible.

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Thanks, just two style stuff for all the tests.

@wbpcode
Copy link
Member Author

wbpcode commented Nov 2, 2020

@htuch It's done. Thanks for your patience and care. 🌹 🌹 🌹
@htuch Sorry, I look @ the wrong person. I still appreciate your suggestions and help during the API review process of this PR. Thanks.

@wbpcode
Copy link
Member Author

wbpcode commented Nov 3, 2020

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13060 (comment) was created by @wbpcode.

see: more, trace.

Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
@wbpcode
Copy link
Member Author

wbpcode commented Nov 3, 2020

@lizan It‘s done now. Thank you very much for your patient review. 👍

@repokitteh-read-only repokitteh-read-only bot removed the api label Nov 3, 2020
@dio dio requested a review from moderation November 3, 2020 11:38
@wbpcode
Copy link
Member Author

wbpcode commented Nov 3, 2020

@moderation Hello, it's almost over now. Hope you can confirm that deps is ok.

project_name = "SkyWalking API",
project_desc = "SkyWalking's language independent model and gRPC API Definitions",
project_url = "https://github.com/apache/skywalking-data-collect-protocol",
version = "8.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

8.2.0 is out at https://github.com/apache/skywalking-data-collect-protocol/releases/tag/v8.2.0. Do you want to bump here or do as a follow up?

Copy link

Choose a reason for hiding this comment

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

8.1.0 and 8.2.0 are the same for the tracing format of the language agent. I think it is fine.
SkyWalking wouldn't break the tracing format for a long time to keep all language agents compatible and LTS.

Copy link

Choose a reason for hiding this comment

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

If this is an envoy project requirement, @wbpcode the 8.1.0->8.2.0 should not break anything you wrote :)

@moderation
Copy link
Contributor

/lgtm deps

I have more accurate release_date checking code now and will review with Harvey upon his return. The new code outputs the following. There are quite a few dates that need to be corrected and I can fix in a follow up.

github_release:  GitHubRelease(organization='apache', project='skywalking-data-collect-protocol', version='v8.1.0', tagged=True)
release_date:  2020-07-31 12:53:06
*WARNING* com_github_apache_skywalking_data_collect_protocol has a newer release than v8.1.0@<2020-07-31 12:53:06>: v8.2.0@<2020-09-08 08:47:35>
com_github_apache_skywalking_data_collect_protocol has a GitHub release date 2020-07-31
An error occurred while processing api/bazel/repository_locations.bzl, please verify the correctness of the metadata: Mismatch with metadata release date of 2020-07-29

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Nov 3, 2020
Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks!

@dio dio merged commit 7d0f89b into envoyproxy:master Nov 4, 2020
jmarantz pushed a commit that referenced this pull request Dec 1, 2020
Commit Message: update docs for skywalking tracer
Additional Description:

Add some docs for SkyWalking tracer. This is a supplement for #13060. It has been delayed until now for personal reasons.

Risk Level: Low
Testing: N/A
Docs Changes: Docs Only
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: wbpcode <comems@msn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants