Add Trace Service Support. by bogdandrutu · Pull Request #345 · envoyproxy/data-plane-api

bogdandrutu · 2017-12-14T01:39:50Z

This is a trace service proposal based on the opencensus (opencensus.io) data model. OpenCensus is an vendor independent tracing (support for zipkin, stackdriver, and soon others) and stats library(prometheus, stackdirver, signalfx, and soon others), integrated in products like gRPC (grpc.io).

One of the usages will be in the Istio (istio.io) environment to push traces to the Mixer.

/cc @douglas-reid @louiscryan

douglas-reid · 2017-12-14T15:54:46Z

+
+// Configuration structure.
+message TraceServiceConfig {
+  // The name of the upstream cluster that hosts the metrics service. The cluster must be


s/metrics/trace ?

mattklein123 · 2017-12-14T19:36:22Z

@bogdandrutu can you potentially give us some overview of what the plans are in this area? Is the plan to implement an Envoy trace driver that speaks this API (easy enough)? I'm mainly trying to understand how this interacts with the OT C++ API (envoyproxy/envoy#2017). In general, I'm a huge fan of the API driven approach so would love to see this move forward.

cc @rnburn @tedsuo

douglas-reid · 2017-12-15T17:27:45Z

@mattklein123 i believe @bogdandrutu just left for vacation and may be a bit slow to respond. I will try to offer my understanding in the meantime.

The plan is to follow this PR with code that enables Envoy to speak this API. The OpenCensus c++ library was just recently released (https://github.com/census-instrumentation/opencensus-cpp). I imagine the Envoy work will involve that library. I'm less clear on the relationship between OT and OpenCensus, and on the details of how the OpenCensus support is anticipated to be added.

bogdandrutu · 2017-12-15T17:51:52Z

@douglas-reid indeed I am on vacation and will respond a bit slower.
@mattklein123 yes we are planning to implement a trace driver that speaks this API, and we believe will not be that hard. OT does not define a data-model (at least last time when I checked) so we cannot use that. OpenCensus tries to offer multiple backend support, currently it offers support for Stackdriver, Zipkin, and we are working with other providers to support them (Dynatrace, Datadog, etc.).

PS: The API that @douglas-reid pointed is in process to become stable, the implementation needs re-work especially for performance point of view but we are committed to do that asap.

mattklein123 · 2017-12-15T18:30:38Z

OK in general this sounds great to me. Please get the PR passing tests. We can discuss implementation details in January.

rakyll · 2017-12-14T20:35:27Z

+message StreamTracesResponse {
+}
+
+message StreamTracesMessage {


s/StreamTracesMessage/StreamingTracesRequest?

rakyll · 2017-12-16T00:44:14Z

+  }
+}
+
+message StreamTracesResponse {


StreamingTracesResponse would be a proper noun phrase.

The names are consistent with https://github.com/envoyproxy/data-plane-api/blob/master/api/metrics_service.proto

mattklein123 · 2017-12-16T00:51:34Z

FYI envoyproxy/envoy#2017 was just merged. IMO it would be nice if the driver was actually an OT C++ driver so it can work in other places also where C++ code is used. We can figure it out in January.

mattklein123 · 2018-01-05T21:53:54Z

@bogdandrutu friendly ping. Can we push this forward? Otherwise I would like to close for now and we can reopen when you are ready?

rakyll · 2018-01-05T22:46:26Z

Bogdan is OOO until mid-January.

mattklein123 · 2018-01-05T22:47:36Z

@rakyll OK can leave it open until @bogdandrutu gets back.

bogdandrutu · 2018-01-16T20:50:03Z

@mattklein123 working on this right now.

bogdandrutu · 2018-01-16T21:24:49Z

@mattklein123 everything is ready.

mattklein123 · 2018-01-16T23:23:10Z

+message TraceServiceConfig {
+  // The name of the upstream cluster that hosts the trace service. The cluster
+  // must be configured in the cluster manager.
+  string cluster_name = 1;


This should switch to the new GrpcService message. (See other examples).

bogdandrutu · 2018-01-16T23:47:04Z

@mattklein123 @htuch @douglas-reid probably one of you has better experience with bazel/proto. I get this error sometimes on the ci:test and cannot reproduce locally. Do you have any idea why?

htuch · 2018-01-17T21:38:26Z

@bogdandrutu this looks like a compiler issue rather than a Bazel one; it's not seeing the issue though.

bogdandrutu · 2018-01-17T23:07:31Z

@htuch the failure is random (sometimes pass, sometimes not), that's why I think I am doing something wrong related to bazel/proto.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bogdandrutu · 2018-01-19T03:04:12Z

Everything looks good.

mattklein123

LGTM. @htuch @wora any comments?

@kyessenov let's merge this and then you can move it around in your PR?

kyessenov · 2018-01-19T18:28:11Z

Sure, let's do that.

wora · 2018-01-20T02:39:11Z

+import "api/grpc_service.proto";
+import "trace.proto";
+
+import "google/api/annotations.proto";


This line is not needed.

wora · 2018-01-20T02:39:41Z

+  // not expect any response to be sent as nothing would be done in the case
+  // of failure.
+  rpc StreamTraces(stream StreamTracesMessage) returns (StreamTracesResponse) {
+  }


Please use ; instead of {}.

wora · 2018-01-20T02:42:16Z

+
+import "validate/validate.proto";
+
+// Service for streaming traces to server that consumes the trace data. It


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.

wora · 2018-01-20T02:43:06Z

+import "validate/validate.proto";
+
+// Service for streaming traces to server that consumes the trace data. It
+// uses OpenCensus data model as a standard to represent trace information.


Remove "as a standard", since it doesn't provide much value.

Please add a link to OpenCensus

wora · 2018-01-20T02:45:45Z

+// Service for streaming traces to server that consumes the trace data. It
+// uses OpenCensus data model as a standard to represent trace information.
+service TraceService {
+  // Envoy will connect and send StreamTracesMessage messages forever. It does


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.

wora · 2018-01-20T02:47:15Z

+message StreamTracesMessage {
+  message Identifier {
+    // The node sending the access log messages over the stream.
+    Node node = 1 [(validate.rules).message.required = true];


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.

wora · 2018-01-20T02:48:01Z

+
+// Configuration structure.
+message TraceServiceConfig {
+  // The upstream gRPC cluster that hosts the metrics service.


Why trace service has anything to do with metrics service? Do they share the schema?

htuch · 2018-07-16T17:49:05Z

Did we ever do anything with this? I see its presence in the repository but not any users.

moderation · 2018-07-16T20:43:11Z

I asked a similar question on Gitter recently - https://gitter.im/census-instrumentation/Lobby?at=5b43d06b9b82c6701bafe6b2. In the responses it looks like the work is being done at the Istio Mixer level (which makes sense) and not at the Envoy level. It looks like https://www.envoyproxy.io/docs/envoy/latest/api-v2/config/trace/v2/trace.proto#config-trace-v2-traceserviceconfig will emit OpenCensus stats to a gRPC endpoint but I'm not really sure.

moderation · 2018-09-12T03:53:49Z

I looked at this again today after the Twitter debate re: OpenCensus & OpenTracing. I'm pretty sure that Trace Service isn't hooked up and can't be used. It is in the documentation at config/trace/v2/trace.proto#config-trace-v2-traceserviceconfig and referred to in proto.

However it isn't defined as a well known name for either tracers source/extensions/tracers/well_known_names.h or stats_sinks stat_sinks/well_known_names.h

After all this I don't know if the metrics service and tracing service are the same thing or different partially implemented things with different names. @ramaraochavali did most the of the metrics service work but I can't find an example of a working config or how this works. It doesn't look like the current metrics service leverages any part of OpenCensus.

I was thinking that if the tracing service worked you could point it to the OpenCensus service and then be able to export traces to multiple backends.

Would be great if @bogdandrutu or @rakyll could chime in. Also good to hear from Istio Policies & Telemetry leads @douglas-reid and @mandarjog

ramaraochavali · 2018-09-12T04:11:36Z

Metrics Service and Tracing Service IMO are different. Metric Service implements gRPC stats sink so that envoy stats can be pushed to the configured end point. It does not deal with OpenCensus at all.
This is how you can configure metrics service

stats_sinks:
  - name: envoy.metrics_service
    config: {grpc_service: {envoy_grpc: {cluster_name: xds_cluster}}}

where xds_cluster points to a static cluster exposing gRPC end point that this services pushes metrics to.

douglas-reid · 2018-09-12T15:52:29Z

@ramaraochavali is correct. The metrics service and the tracing service are different. The original intent of this PR was solely to provide an API for Envoy to stream out trace spans to a backend. OpenCensus obviously supports stats/metrics and traces, but this was focused exclusively on trace data.

I cannot speak with any authority on the OpenCensus' teams priorities around implementation. At the time of the original PR, I understood that work based on their recently released code would immediately follow, hooking up this API. Sometime soon after, priorities shifted I guess. As @moderation notes, there have been Istio Mixer PRs that have added support for generating trace spans from Report calls. This should enable export of traces to any trace backend, based on request/response metadata, for Istio use cases.

I don't know if the OpenCensus team wants to continue pursuing trace export via Envoy or not. I think it would be useful. The last I heard, the OpenCensus folks were working on adding support for stats export from Envoy however. It would be good to have confirmation of that effort.

g-easy · 2018-09-12T17:53:57Z

OpenCensus contributor here. We're looking into supporting this and will come back with a more concrete proposal soon.

bogdandrutu force-pushed the opencensus branch 4 times, most recently from 1d08b2a to e5deb87 Compare December 14, 2017 03:05

douglas-reid reviewed Dec 14, 2017

View reviewed changes

bogdandrutu force-pushed the opencensus branch from e5deb87 to 0541f70 Compare December 14, 2017 18:28

rakyll reviewed Dec 16, 2017

View reviewed changes

bogdandrutu force-pushed the opencensus branch from 0541f70 to 517b88e Compare January 16, 2018 20:49

mattklein123 reviewed Jan 16, 2018

View reviewed changes

bogdandrutu force-pushed the opencensus branch from 258cd61 to db6b1e9 Compare January 16, 2018 23:39

Add Trace Service Support.

ba0a3f1

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bogdandrutu force-pushed the opencensus branch from db6b1e9 to ba0a3f1 Compare January 18, 2018 21:05

mattklein123 approved these changes Jan 19, 2018

View reviewed changes

htuch approved these changes Jan 19, 2018

View reviewed changes

mattklein123 merged commit aae0051 into envoyproxy:master Jan 19, 2018

wora reviewed Jan 20, 2018

View reviewed changes

moderation mentioned this pull request Sep 30, 2018

Integrate with OpenCensus stats collection and distributed tracing framework envoyproxy/envoy#2456

Closed


		import "validate/validate.proto";

		// Service for streaming traces to server that consumes the trace data. It

Conversation

bogdandrutu commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Dec 14, 2017

Uh oh!

douglas-reid commented Dec 15, 2017

Uh oh!

bogdandrutu commented Dec 15, 2017

Uh oh!

mattklein123 commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Dec 16, 2017

Uh oh!

mattklein123 commented Jan 5, 2018

Uh oh!

rakyll commented Jan 5, 2018

Uh oh!

mattklein123 commented Jan 5, 2018

Uh oh!

bogdandrutu commented Jan 16, 2018

Uh oh!

bogdandrutu commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu commented Jan 16, 2018

Uh oh!

htuch commented Jan 17, 2018

Uh oh!

bogdandrutu commented Jan 17, 2018

Uh oh!

bogdandrutu commented Jan 19, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Jul 16, 2018

Uh oh!

moderation commented Jul 16, 2018

Uh oh!

moderation commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramaraochavali commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglas-reid commented Sep 12, 2018

Uh oh!

g-easy commented Sep 12, 2018

Uh oh!

moderation commented Sep 12, 2018 •

edited

Loading

ramaraochavali commented Sep 12, 2018 •

edited

Loading