You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/12/16 11:32:41 UTC
[GitHub] [skywalking] Shikugawa opened a new issue #6019: Add package definition on data collection protocol
Shikugawa opened a new issue #6019:
URL: https://github.com/apache/skywalking/issues/6019
Please answer these questions before submitting your issue.
- Why do you submit this issue?
- [ ] Question or discussion
- [ ] Bug
- [x] Requirement
- [ ] Feature or performance improvement
___
### Question
- What do you want to know?
___
### Bug
- Which version of SkyWalking, OS, and JRE?
- Which company or project?
- What happened?
If possible, provide a way to reproduce the error. e.g. demo application, component version.
___
### Requirement or improvement
- Please describe your requirements or improvement suggestions.
This requirement had derived from this issue. https://github.com/SkyAPM/cpp2sky/issues/36
In short, we should add package definition to the proto of data collection protocol, to avoid conflict between existing class definition like `Commands`. This feature will cause breaking change so we should discuss carefully. My consideration is to introduce proxy layer to rewrite `:path`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747204691
> make a compat service that have same signature in server-side only, keep with current implementation.
This seems needing a little more coding than generating directly, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746396012
I knew if there could be conflict in the user codes. But in java, dotnet, even go, there is some package name options used today to avoid this issue. I don't know what are available for other languages.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] lizan commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
lizan commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747202192
No, both version of server using same proto of the data, so it only handles different version of URL on the wire.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746368691
> What is the impact for current service?
If the users code space contains class whose name is the same as what we defined in the protocol, it conflicts (maybe won't compile?), this was exactly what I encountered in the Python agent.
In the Python agent, we solve this by generating the codes, and modifying its namespace (top package name in Python's term) to `skywalking`, https://github.com/apache/skywalking-python/blob/master/tools/codegen.py
I am not sure whether this is feasible in C++.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] Shikugawa commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
Shikugawa commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747267886
@kezhenxu94 I can tolerate it. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747407642
I have asked to make the package name shorter `package skywalking.v3;`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] lizan commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
lizan commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747213504
Yeah sure, let's have a separate branch in data-collection-protocol, and point cpp2sky there?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746399127
I mean this, https://github.com/apache/skywalking-data-collect-protocol/blob/master/language-agent/Tracing.proto#L21
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747204029
> we should be able to add package to data messages, this doesn't break wire compatibility (unless we use protobuf.Any, which I don't think this is the case).
In this case, I think it is better for cpp sdk hosts its own and modified trace.proto and common.proto? And verified by e2e with skywalking 8.3.0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747264877
@wu-sheng maybe only keep the prefix `org.apache.skywalking.v3` to be `org.apache.skywalking.v3.SegmentObject`? @Shikugawa is that short enough?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747270322
> @kezhenxu94 I can tolerate it. Thanks.
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] lizan commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
lizan commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747207666
> In this case, I think it is better for cpp sdk hosts its own and modified trace.proto and common.proto? And verified by e2e with skywalking 8.3.0
That can happen in data-collection-protocol directly and won't break anything? The problem is the service package, and it's depends on trace.proto and common.proto. Hosting only those two doesn't solve the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng closed issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng closed issue #6019:
URL: https://github.com/apache/skywalking/issues/6019
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747260459
Hey @Shikugawa , I pushed a branch with package in the message, can you take a look? And I'll test the others in other agents and the main repo
https://github.com/apache/skywalking-data-collect-protocol/tree/add-pkg-name
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746447337
I mean they will ask. Anyway, this thing makes sense to me, but the requirement will make the OAP has to adopt service with and without package name, if they are not compatible with previous versions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747208550
I mean, you said only change message package name would not break anything, right? How about we try this first?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] Shikugawa commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
Shikugawa commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747201445
> > Quick conclusion: adding package name changes the service name, that breaks all existing agents, I'm still wondering whether we could solve it in cpp2sky side, but package name definitely needs to be added someday when we decide to have break changes
>
> There are ways to provide this compatibility:
>
> 1. we should be able to add package to data messages, this doesn't break wire compatibility (unless we use protobuf.Any, which I don't think this is the case).
> 2. make a compat service that have same signature in server-side only, keep with current implementation.
> 3. add package to protobuf service, which shares the underlying implementation with 2.
>
> In this way the server would be able to handle both version of service, and it won't break wire compatibility.
Does it means to provide interception layer on the OAP that handles both current message and next version message (with package) ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747168884
Quick conclusion: adding package name changes the service name, that breaks all existing agents, I'm still wondering whether we could solve it in cpp2sky side, but package name definitely needs to be added someday when we decide to have break changes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 edited a comment on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747264877
What about only keeping the prefix `org.apache.skywalking.v3` to be `org.apache.skywalking.v3.SegmentObject`? @Shikugawa is that short enough?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] Shikugawa commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
Shikugawa commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747174214
@wu-sheng Sure. It would be C++ dedicated problem and no critical solution to avoid this. Maybe in this case, users should use original namespace on thair application. Fortunately, Envoy do that and no problem had caused. https://github.com/envoyproxy/envoy/blob/master/source/extensions/tracers/skywalking/skywalking_types.h#L11
But, to determine package name could be good following with other training software. thank you for discussion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 edited a comment on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746368691
> What is the impact for current service?
If the users code space contains class whose name is the same as what we defined in the protocol, it conflicts (maybe won't compile?), this was exactly what I encountered in the Python agent.
In the Python agent, we solve this by generating the codes, and modifying its namespace (top package name in Python's term) to `skywalking`
https://github.com/apache/skywalking-python/blob/f7f5aa77ace3f640fed3acaed9b7325a72c40a35/tools/codegen.py#L43-L49
I am not sure whether this is feasible in C++.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746443023
> I knew if there could be conflict in the user codes. But in java, dotnet, even go, there is some package name options used today to avoid this issue. I don't know what are available for other languages.
There is no option for Python https://developers.google.com/protocol-buffers/docs/reference/python-generated#package hence we have to do it on our own.
And seems there is no dedicated `option` for C++, C++ uses `package` option as per https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#package
> The conflict is tricky if those proto files have to generate with other protos, like envoy, AFAIK.
I checked, proto files from Envoy, OpenCensus, Jaeger all have an `package` declaration.
https://github.com/apache/skywalking/blob/08b31adf60fee5452574e3a45945d9afb457083a/oap-server/server-receiver-plugin/receiver-proto/src/main/proto/jaeger/model.proto#L17
https://github.com/apache/skywalking/blob/08b31adf60fee5452574e3a45945d9afb457083a/oap-server/server-receiver-plugin/receiver-proto/src/main/proto/opencensus/proto/resource/v1/resource.proto#L17
https://github.com/apache/skywalking/blob/08b31adf60fee5452574e3a45945d9afb457083a/oap-server/server-receiver-plugin/receiver-proto/src/main/proto/prometheus/client_model/metrics.proto#L16
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746353405
What is the impact for current service? if it makes things incompatible, then it is hard to ask all agents and users to upgrade in short term.
In most languages, there are already namespace for generated codes, the skywalking proto will generate codes separately with user codes. Does cpp have any solution about this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] lizan commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
lizan commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747194576
> Quick conclusion: adding package name changes the service name, that breaks all existing agents, I'm still wondering whether we could solve it in cpp2sky side, but package name definitely needs to be added someday when we decide to have break changes
There are ways to provide this compatibility:
1. we should be able to add package to data messages, this doesn't break wire compatibility (unless we use protobuf.Any, which I don't think this is the case).
2. make a compat service that have same signature in server-side only, keep with current implementation.
3. add package to protobuf service, which shares the underlying implementation with 2.
In this way the server would be able to handle both version of service, and it won't break wire compatibility.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747409792
A new PR will be supported by @kezhenxu94 to recheck the compatibility with other existing agents.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747170186
@Shikugawa From the discussion, I think the cpp sdk would share the grpc client with others, so, once the compiling passed, there would never have a conflict on the runtime.
Please correct me if I got something wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] Shikugawa edited a comment on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
Shikugawa edited a comment on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747174214
@wu-sheng Sure. It would be C++ dedicated problem and no critical solution to avoid this. Maybe in this case, users should use original namespace on thair application. Fortunately, Envoy do that and no problem had caused. https://github.com/envoyproxy/envoy/blob/master/source/extensions/tracers/skywalking/skywalking_types.h#L11
But, to determine package name could be good following with other tracing softwares. thank you for discussion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-746400549
The conflict is tricky if those proto files have to generate with other protos, like envoy, AFAIK.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] lizan commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
lizan commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747197238
FYI this is a dup of https://github.com/apache/skywalking/issues/5266 that we've discussed before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] Shikugawa commented on issue #6019: Add package definition on data collection protocol
Posted by GitBox <gi...@apache.org>.
Shikugawa commented on issue #6019:
URL: https://github.com/apache/skywalking/issues/6019#issuecomment-747263715
@kezhenxu94 Thank you for your working. LGTM but I have pretty worry in this. The package name seems mostly jvm language way. It will cause on C++ like too-long namespace, such as
```
::grpc::ClientWriterInterface< ::org::apache::skywalking::apm::network::language::agent::v3::SegmentObject>
```
Can you shorten this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org