-
Notifications
You must be signed in to change notification settings - Fork 274
Add Trace Service Support. #345
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| // [#proto-status: draft] | ||
|
|
||
| package envoy.api.v2; | ||
|
|
||
| import "api/base.proto"; | ||
| import "api/grpc_service.proto"; | ||
| import "trace.proto"; | ||
|
|
||
| import "google/api/annotations.proto"; | ||
|
|
||
| import "validate/validate.proto"; | ||
|
|
||
| // Service for streaming traces to server that consumes the trace data. It | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sentence can be simplified: An interface for streaming traces from a client to a server. There is no need to mention "consumes the trace data". The server can just forward the data elsewhere. There is no need to speculate the usage. |
||
| // uses OpenCensus data model as a standard to represent trace information. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove "as a standard", since it doesn't provide much value. Please add a link to OpenCensus |
||
| service TraceService { | ||
| // Envoy will connect and send StreamTracesMessage messages forever. It does | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This interface can be used by different client, there is no need to mention Envoy. "forever" is also too strong. How about: // The client will stream trace to the server continuously until either the client or the server terminates the stream. |
||
| // not expect any response to be sent as nothing would be done in the case | ||
| // of failure. | ||
| rpc StreamTraces(stream StreamTracesMessage) returns (StreamTracesResponse) { | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use |
||
| } | ||
|
|
||
| message StreamTracesResponse { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. StreamingTracesResponse would be a proper noun phrase.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The names are consistent with https://github.com/envoyproxy/data-plane-api/blob/master/api/metrics_service.proto |
||
| } | ||
|
|
||
| message StreamTracesMessage { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/StreamTracesMessage/StreamingTracesRequest?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto. |
||
| message Identifier { | ||
| // The node sending the access log messages over the stream. | ||
| Node node = 1 [(validate.rules).message.required = true]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you put the trace data into a data, the key should always be a string, the client should normalize the id into a string instead of asking server to do it. |
||
| } | ||
|
|
||
| // Identifier data effectively is a structured metadata. | ||
| // As a performance optimization this will only be sent in the first message | ||
| // on the stream. | ||
| Identifier identifier = 1; | ||
|
|
||
| // A list of Span entries | ||
| repeated opencensus.proto.trace.Span spans = 2; | ||
| } | ||
|
|
||
| // Configuration structure. | ||
| message TraceServiceConfig { | ||
| // The upstream gRPC cluster that hosts the metrics service. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why trace service has anything to do with metrics service? Do they share the schema? |
||
| GrpcService grpc_service = 1 [(validate.rules).message.required = true]; | ||
| } | ||
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.
This line is not needed.