Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
tracer: Add SkyWalking tracer #13060
Changes from 48 commits
74e0892
f384820
b8ce5a3
f95f9dd
f44d946
bf033d8
348387e
d689a88
3dcb800
5583faf
bd16737
44a954f
6bef0f4
7c05904
7c80783
e57eca8
86da0e7
a88e92d
7b1cee9
0156a82
0a8d5e7
40150ee
8a15758
1ae2902
fe02559
c7c148e
a8ca0fc
4184636
647679e
e10a47b
c914203
75f5ed7
0952632
b518d3e
73a7f2d
3373ab8
c58cf8f
f76ea47
33d9e61
46a82ab
c63db99
57348a3
1adcbb2
9804a03
73cd767
b622945
641d546
ada59bc
0a659b3
c3fa2d9
c705f4e
69d7dd1
998c8a1
fbc6b93
056c459
82ef1fe
f80844a
7488a66
e4a815a
d9a1278
c3b30c8
b2e8661
753f79f
32ba207
749f69a
ec03cb7
8fa92a2
fdc58a0
b93d3ff
675f634
f95b77d
6aab97c
fe43441
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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?
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.
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.
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.
If this is an envoy project requirement, @wbpcode the 8.1.0->8.2.0 should not break anything you wrote :)
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 if it's a Google gRPC service without cluster name?
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.
The cluster name here refers to the cluster name in Envoy localinfo (configured by node.cluster in bootstrap or --service-cluster). It is used to indicate the identity of Envoy when reporting data to the skywalking server when
service_name
is empty. I will update this comment to explain it more clearly.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.
@htuch But you reminded me that the cluster name in localinfo may also be empty. Here I will make a modification. If the cluster name is also empty (which will not happen in most cases), use
EnvoyProxy
as the service name.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.
Yeah, please clarify that you mean "local cluster name". Is
EnvoyProxy
some standard? Is there a reason empty string won't work?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.
@htuch
EnvoyProxy
is not standard but a choice. It is component name for Envoy in the Skywalking (https://github.com/wbpcode/skywalking/blob/129effe4e12211fcccdc40249db896728457dc21/oap-server/server-bootstrap/src/main/resources/component-libraries.yml) So I choseEnvoyProxy
as the default name when no any configuration is available. In practice, I have almost never encountered a situation where the local cluster name and local node name are empty. The control plane always generates these two configurations for Envoy. So I think this default name only exists as an insurance and almost have no chance of being actually used.Service name and instance name are indispensable when creating the SkyWalking propagation header.
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 explaining, makes sense.
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.
there were previous comment on the naming,
authentication
seems to broad and ambiguous,token
is better 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.
What is the upper bound on segment size?
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.
Each segment is composed of multiple spans. In most cases, it contains only two spans (one entry span and one exit span). There is currently no restriction on the size of a single segment itself.
This configuration is only used to limit the maximum number of segments temporarily stored in memory when a network failure occurs. Segments exceeding this number will be discarded.
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, I think by "backend server" you mean the SkyWalking backend service. It would be helpful to clarify that, so as not to confuse with the upstream.
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.
The
import public
around this proto is just for backward compatibility during re-structuring proto files, new proto doesn't have to be 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.
How about
backend_token
. Because how the authentication active is based on skywalking backend setting.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
@phlax for sandbox review.