Add agent/common proto and BUILD.#81
Conversation
| PHP = 6; | ||
| PYTHON = 7; | ||
| RUBY = 8; | ||
| UNKNOWN = 9; |
There was a problem hiding this comment.
UNKNOWN has to be the first one and = 0. See https://developers.google.com/protocol-buffers/docs/proto3#enum look for default value.
| } | ||
|
|
||
| // Identifier that uniquely identifies a process within a VM/container. | ||
| // In the future we plan to extend the identifier proto definition to support |
There was a problem hiding this comment.
This comment should be for the Node not here. Also service name is already supported.
| uint32 pid = 2; | ||
|
|
||
| // Represented in epoch time. | ||
| google.protobuf.Timestamp timestamp = 3; |
There was a problem hiding this comment.
s/timestamp/start_timestamp?
| option java_package = "io.opencensus.proto.agent.common.v1"; | ||
| option java_outer_classname = "CommonProto"; | ||
|
|
||
| option go_package = "github.com/census-instrumentation/opencensus-proto/gen-go/agentproto/commonproto/v1"; |
There was a problem hiding this comment.
Sounds good, I would prefer to leave the name as-is, since in other protos the Go package names are similar, e.g:
"github.com/census-instrumentation/opencensus-proto/gen-go/traceproto"
There was a problem hiding this comment.
I think we should change all the package including the one you showed:
"github.com/census-instrumentation/opencensus-proto/gen-go/trace/v1"
"github.com/census-instrumentation/opencensus-proto/gen-go/agent/trace/v1"
| map<string, string> attributes = 3; | ||
|
|
||
| // Additional informantion on service. | ||
| ServiceInfo service_info = 4; |
There was a problem hiding this comment.
nit: move service_info before attributes.
| message LibraryInfo { | ||
|
|
||
| enum Language { | ||
| UNKNOWN = 0; |
There was a problem hiding this comment.
To be consistent with other enums in this repo maybe consider LANGUAGE_UNSPECIFIED
| // Additional informantion on service. | ||
| ServiceInfo service_info = 4; | ||
|
|
||
| // TODO(songya): Add more identifiers in the future as needed, like cloud identifiers. |
There was a problem hiding this comment.
Keep the line length to 80 characters.
|
@songy23 I think you can merge this PR (maybe include my suggestion or not) and we can get the feedback later. don't want to block you on that. |
|
Thanks! |
Separated from #79.