Add agent trace service proto definitions. by songy23 · Pull Request #79 · census-instrumentation/opencensus-proto

songy23 · 2018-08-25T00:43:12Z

Also add java_grpc_library build rule so that the generated gRPC service can work with Java-Bazel build.

I feel it's better to put the proto definitions under opencensus-proto, and use the gen-go files in the agent/service repo.

Next step: update the gen-go files.

/cc @rakyll @SergeyKanzhelev @lmolkova @draffensperger

odeke-em · 2018-08-25T01:46:12Z

opencensus/proto/agent/trace/v1/trace_service.proto

+option java_package = "io.opencensus.proto.agent.trace.v1";
+option java_outer_classname = "TraceServiceProto";
+
+option go_package = "github.com/census-instrumentation/opencensus-proto/gen-go/traceserviceproto";


How come the Java package is versioned by the Go one isn't versioned?

Good catch - that's a typo made by me. Fixed.

bogdandrutu · 2018-08-25T15:35:37Z

opencensus/proto/agent/trace/v1/trace_service.proto

+  // TODO(songya): add rate limiting sampler.
+}
+
+message Node {


This should go in a agent.common.v1 namespace because it will be shared.

Make sense to me.

bogdandrutu · 2018-08-25T15:36:30Z

opencensus/proto/agent/trace/v1/trace_service.proto

+// Identifier that uniquely identifies a process within a VM/container.
+// In the future we plan to extend the identifier proto definition to support
+// additional information (e.g cloud id, monitored resource, service name, etc.)
+message ProcessIdentifier {


same for this, should be in a common package.

bogdandrutu · 2018-08-25T16:04:17Z

opencensus/proto/agent/trace/v1/trace_service.proto

+message TraceConfig {
+
+  // The global default Sampler.
+  Sampler sampler = 1;


oneof sampler {
ProbabilityConfig
AlwaysSampleConfig
NeverSampleConfig
RateLimitingConfig
}

bogdandrutu · 2018-08-25T17:22:43Z

opencensus/proto/agent/trace/v1/trace_service.proto

+  // TODO(songya): add rate limiting sampler.
+}
+
+message Node {


I think also would be good to have in the Node a:
message LibraryInfo {
enum Language {
UNKNOWN = 0;
GO_LANG = 1;
JAVA = 2;
...
}
Language language = 1; // we can also use string here. open for opinions here.
string version = 2;
}

Sounds good, added. (I would prefer to use an Enum here.)

lmolkova · 2018-08-27T18:35:20Z

opencensus/proto/agent/common/v1/common.proto

+message ProcessIdentifier {
+
+  // The host name.
+  string host_name = 1;


what is a good example of host_name: machine/container name/ip address? Service name?

Currently we use machine/container name, added some example.

Thanks for the explanation. I was thinking about it more and it seems service name is kind of essential: e.g. zipkin and jaeger exporters explicitly ask for it, stackdriver exporters get it from env var and AppInsights also heavily relies on it. Since everyone needs it, can we have service name right away?

That makes sense to me, we can add another service_name field to Node. WDYT @odeke-em @bogdandrutu

I think we should have a "ServiceInfo" message which currently contains just "name", this way we can extend more the message if needed. I can think of version, build informations and others.

SG, added a ServiceInfo message.

lmolkova · 2018-08-27T18:35:54Z

opencensus/proto/agent/common/v1/common.proto

+message LibraryInfo {
+
+  enum Language {
+    UNKNOWN = 0;


can we add python now, and I'll be using it right away in the python exporter?

Sure, added.

bogdandrutu · 2018-08-27T19:45:53Z

opencensus/proto/agent/trace/v1/trace_service.proto

+    RateLimitingSampler rate_limiting_sampler = 4;
+  }
+
+  // TODO(songya): add more fields and move this to trace.proto.


We should put it in a config.proto in the trace package. Can you do that?

bogdandrutu

I think this PR got a bit longer than expected. Lets split it in 3:

trace_config;
agent/common;
agent/trace;

I think it is harder to follow changes everywhere.

bogdandrutu · 2018-08-27T21:02:34Z

opencensus/proto/agent/common/v1/common.proto

+    PYTHON = 3;
+    RUBY = 4;
+    PHP = 5;
+    C_SHARP = 6;


Please use alphabetical order to avoid any discussion about ordering here.

bogdandrutu · 2018-08-27T21:04:14Z

opencensus/proto/agent/common/v1/common.proto

+  ProcessIdentifier identifier = 1;
+
+  // Information on the OpenCensus Library who initiates the stream.
+  LibraryInfo LibraryInfo = 2;


Please use the proto style guide. variable names are not like java (cpp style is used): library_info

Got it, fixed.

bogdandrutu · 2018-08-27T21:04:45Z

opencensus/proto/agent/common/v1/common.proto

+  string name = 1;
+
+  // Version of the service.
+  string version = 2;


Version was just an example. Please don't add it for the moment.

bogdandrutu · 2018-08-27T21:09:13Z

opencensus/proto/trace/trace_config.proto

+  oneof sampler {
+    ProbabilitySampler probability_sampler = 1;
+
+    AlwaysSampler always_sampler = 2;


I think something like "ConstantSampler constant_sampler = 2;" can replace always/never and we have ConstantSampler {
boolean decision = 1;
}

This way we do not have empty sampler messages and I think will look better.

Make sense to me.

songy23

Post #80 for trace_config and #81 for agent/common.

songy23 · 2018-08-27T21:28:00Z

opencensus/proto/trace/trace_config.proto

+  oneof sampler {
+    ProbabilitySampler probability_sampler = 1;
+
+    AlwaysSampler always_sampler = 2;


Make sense to me.

songy23 · 2018-08-27T21:34:51Z

opencensus/proto/agent/common/v1/common.proto

+  ProcessIdentifier identifier = 1;
+
+  // Information on the OpenCensus Library who initiates the stream.
+  LibraryInfo LibraryInfo = 2;


Got it, fixed.

songy23 · 2018-08-27T21:35:01Z

opencensus/proto/agent/common/v1/common.proto

+    PYTHON = 3;
+    RUBY = 4;
+    PHP = 5;
+    C_SHARP = 6;


songy23 · 2018-08-27T21:38:09Z

opencensus/proto/agent/common/v1/common.proto

+  string name = 1;
+
+  // Version of the service.
+  string version = 2;


songy23 · 2018-08-27T21:44:11Z

Please review #80 and #81 first, I'll rebase and update this PR once those two are merged.

bogdandrutu · 2018-08-27T23:19:12Z

opencensus/proto/agent/trace/v1/trace_service.proto

+}
+
+message ConfigTraceServiceRequest {
+  // Identifier data effectively is a structured metadata. This is required only in the first message on the stream.


Use 80 characters everywhere.

Fixed locally, will rebase and push after merging #81.

songy23 · 2018-08-28T16:38:14Z

Rebased, PTAL.

bogdandrutu · 2018-08-28T22:01:45Z

opencensus/proto/agent/trace/v1/BUILD.bazel

+
+load("@grpc_java//:java_grpc_library.bzl", "java_grpc_library")
+
+proto_library(


I think we should also have the go generated using bazel rules:
https://github.com/bazelbuild/rules_go/blob/master/proto/core.rst#example-grpc

I think we're directly depending on the gen-go files in Go? @rakyll

Adding go_proto_library build rule will introduce a lot of transitive dependencies.

I left a TODO here for adding the go_proto_library rule if needed. That will be a lot of code changes so I think it's better to do it in a separate PR.

songy23 · 2018-08-28T23:51:27Z

Thanks!

songy23 requested review from bogdandrutu and odeke-em August 25, 2018 00:43

odeke-em reviewed Aug 25, 2018

View reviewed changes

bogdandrutu reviewed Aug 25, 2018

View reviewed changes

bogdandrutu suggested changes Aug 25, 2018

View reviewed changes

songy23 force-pushed the agent branch from 2be4dc2 to 69f3575 Compare August 25, 2018 22:19

lmolkova reviewed Aug 27, 2018

View reviewed changes

lmolkova approved these changes Aug 27, 2018

View reviewed changes

bogdandrutu reviewed Aug 27, 2018

View reviewed changes

songy23 mentioned this pull request Aug 27, 2018

Add agent/common proto and BUILD. #81

Merged

songy23 commented Aug 27, 2018

View reviewed changes

songy23 added the DO NOT MERGE label Aug 27, 2018

census-instrumentation deleted a comment from songy23 Aug 27, 2018

bogdandrutu reviewed Aug 27, 2018

View reviewed changes

songy23 force-pushed the agent branch 2 times, most recently from 3eef94e to ef3af70 Compare August 28, 2018 16:34

songy23 removed the DO NOT MERGE label Aug 28, 2018

songy23 changed the title ~~Start adding proto definitions for Agent protocol.~~ Add agent trace service proto definitions. Aug 28, 2018

songy23 added 2 commits August 28, 2018 14:00

Add agent trace service proto definitions.

b010605

Update BUILD and import.

6d117a9

songy23 force-pushed the agent branch from ef3af70 to 6d117a9 Compare August 28, 2018 21:05

bogdandrutu approved these changes Aug 28, 2018

View reviewed changes

Leave a TODO for adding go_proto_library rule.

a287ab6

bogdandrutu approved these changes Aug 28, 2018

View reviewed changes

songy23 self-assigned this Aug 28, 2018

songy23 merged commit 3d9e99d into census-instrumentation:master Aug 28, 2018

lmolkova mentioned this pull request Aug 30, 2018

Initial trace exporter implementation for opencensus agent census-instrumentation/opencensus-python#266

Merged


		load("@grpc_java//:java_grpc_library.bzl", "java_grpc_library")

		proto_library(

Conversation

songy23 commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

lmolkova Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 left a comment

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!

songy23 commented Aug 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 commented Aug 25, 2018 •

edited

Loading

lmolkova Aug 27, 2018 •

edited

Loading