Add GrpcSender for jaeger-collector #169
Conversation
|
Once #167 is merged, I will also add cross dock tests. For now, I only tested it manually against jaeger-collector in jaegertracing/all-in-one. |
Falco20019
changed the title
Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
Falco20019
force-pushed
the
Falco20019/grpc-sender-2
branch
from
ac25212 to
8a4ba61
Compare
Signed-off-by: Kraemer, Benjamin <[email protected]>
Falco20019
force-pushed
the
Falco20019/grpc-sender-2
branch
from
8a4ba61 to
dc7152f
Compare
|
I left the conversations open for you to easier find the content. I resolved all but the |
Falco20019
changed the title
Protos will be generated from jaeger-idl directly Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
- Use Jaeger.Communication.Grpc instead Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
Signed-off-by: Kraemer, Benjamin <[email protected]>
|
@yurishkuro I fixed all of the remaining topics. The client ca is also now using a string and should match the usage of Once this and #184 are merged, I will release |
| @@ -0,0 +1,291 @@ | |||
| // <auto-generated> | |||
There was a problem hiding this comment.
Kinda sad that this is being generated into the library, I was hoping gogo annotations, since they only apply to Go generator, would be no-op elsewhere.
There was a problem hiding this comment.
Sadly not. And it's not even possible to generate it without manually removing them. That was the first approach... But hard to maintain.
| /// <summary> | ||
| /// The root certificate file used to check the server side certificate from GRPC collector (roots.pem). | ||
| /// </summary> | ||
| public const string JaegerGrpcRootCertificate = JaegerPrefix + "GRPC_ROOT_CERTIFICATE"; |
There was a problem hiding this comment.
Q: is it, strictly speaking, scoped to gRPC only? Wouldn't it be possible to use the same certificates with HTTPS?
There was a problem hiding this comment.
You are technically right. But right now, it's only used there. Do we have environment variables for HTTPS in other clients? If yes, I would try to keep it in sync.
There was a problem hiding this comment.
I don't believe mTLS is supported in any other clients.
There was a problem hiding this comment.
Then I will for now just merge it. For 1.0.0 we might look into it again.
Which problem is this PR solving?
Fixes Support for GrpcSender #139
Supersedes Add GrpcSender for jaeger-collector #141
WIP until Added crossdock integration tests #167 is merged and code is checked
Short description of the changes
Thrift. This add a separate packageJaeger.Senders.Grpc(likeJaeger.Senders.Thrift).This can be used with
Jaeger.Coreto avoid having a dependency onThrift. It includes no jaeger-agent sender and makes this also clear in the documentation. In comparison to #141, this also now supportsMaxPacketSizelike withUdpSender.