Envoy: Envoy protos should not have a dependency on opencensus proto

[sanjaypujare]

WARNING: If you want to report crashes, leaking of sensitive information,
and/or other security issues, please consider
reporting them using appropriate channels.

Bug Template

Title: Envoy protos should not have a dependency on opencensus proto

Description:
We use Envoy protos for using the xDS APIs between our clients and management server. Until recently things were okay but now there is a dependency on opencensus/proto/trace/v1/trace_config.proto which is unfortunate. The dependency exists because envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto includes third_party/envoy/src/api/envoy/config/trace/v2/trace.proto which now includes third_party/opencensus_proto/trace/v1/trace_config.proto . I think http_connection_manager.proto is a common file included from multiple "core" files.

This has started happening recently probably with the PR https://github.com/envoyproxy/envoy/pull/10324
Repro steps:
Without installing the opencensus protos when I try to compile base.proto I get the following compile errors:

Execution failed for task ':grpc-xds:generateProto'.
  opencensus/proto/trace/v1/trace_config.proto: File not found.
  envoy/config/trace/v2/trace.proto:11:1: Import "opencensus/proto/trace/v1/trace_config.proto" was not found or had errors.
  envoy/config/trace/v2/trace.proto:168:3: "opencensus.proto.trace.v1.TraceConfig" is not defined.
  envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto:10:1: Import "envoy/config/trace/v2/trace.proto" was not found or had errors.

[sanjaypujare]

@dapengzhang0 can add more comments.

[htuch]

@yskopets @g-easy do you have any thoughts here?

I'm wondering why it is not acceptable to have additional proto dependencies for HCM? I.e. what is the criteria.

@sanjaypujare I imagine this is in the context of gRPC, doesn't gRPC depend on OpenCensus anyway?

[sanjaypujare]

I'm wondering why it is not acceptable to have additional proto dependencies for HCM? I.e. what is the criteria.

Couple of things:
1) compiling the protos becomes more difficult because of the additional dependency
2) should envoy depend on a particular metric and tracing API?

@sanjaypujare I imagine this is in the context of gRPC, doesn't gRPC depend on OpenCensus anyway?

But not on opencensus protos. Even the dependency on opencensus libraries has been removed recently IIRC. Will add a reference here.

[sanjaypujare]

For grpc-java https://github.com/grpc/grpc-java/pull/6577 talks about "dependency on opencensus is at runtime by reflection". Even without this we never had any dependency on opencensus protos.

[dapengzhang0]

Why should trace.proto explicitly depend on a particular tracing provider third_party/opencensus_proto/trace/v1/trace_config.proto? Could it use Any type for the trace config and let the implementation select the tracing provider and apply the config dynamically in the runtime?

[htuch]

I think Any should be fine here, @yskopets WDYT?

[g-easy]

Part of Envoy's config depends on opencensus-proto's trace_config.proto. We could change the Envoy config to not do this.

However, opencensus-cpp's ocagent exporter also depends on opencensus-proto.
(likewise, our Stackdriver exporter depends on googleapis)

gRPC doesn't strictly depend on OpenCensus, there's an optional grpc-opencensus "plugin" that bridges the two but Envoy doesn't use it.

[yskopets]

@htuch

Sorry, I didn't notice this question in time.

I think, the issue here is not that Tracing.OpenCensusConfig message explicitly depends on opencensus.proto.trace.v1.TraceConfig, but rather that dependency HttpConnectionManager => Tracing.Http pulls in Tracing.OpenCensusConfig.

Specifically, Tracing.Http is already defined using Any:

message Tracing {
  message Http {
    string name = 1 [(validate.rules).string = {min_bytes: 1}];

    oneof config_type {
      google.protobuf.Any typed_config = 3;
    }
}

And HttpConnectionManager only depends on Tracing.Http.

So, it should be possible to compile HCM without OpenCensus.

I think, the right solution here is to move Tracing.OpenCensusConfig (and other providers) out of the *.proto file with Tracing.Http.

Regarding the dependency Tracing.OpenCensusConfig => opencensus.proto.trace.v1.TraceConfig, I think that replacing an explicit dependency with dependency on Any hurts expressiveness of the API.

If you're not planning to use OpenCensus, then don't compile Tracing.OpenCensusConfig.

If you choose to compile Tracing.OpenCensusConfig, then it won't have much value without opencensus.proto.trace.v1.TraceConfig.

[yskopets]

@htuch btw, why does configuration for tracing provider implementations live in config.trace rather than in extensions.tracers.<provider> ?

[sanjaypujare]

@htuch
...
So, it should be possible to compile HCM without OpenCensus.

That's the change being requested. Who can make that change?

[mattklein123]

@sanjaypujare +1 to @yskopets proposal. Optimally each tracer config should be in its own proto file and in the extension directory. Can we make that happen please?

[sanjaypujare]

@mattklein123 we have a workaround for our issue so this is no longer a blocker for us although we would prefer to have this resolved. Not sure I can get to it soon. How about @yskopets or @g-easy or the person who introduced the dependency?

[yskopets]

@sanjaypujare 🙂ok, I'll see what I can do.

[sanjaypujare]

@yskopets thanks.

[htuch]

Yes, agreed, we should move trace providers to reflect the regular extension structure.

[htuch]

@yskopets FWIW, it's totally fine to move the protos to distinct files inside existing packages within a major version; this doesn't constitute a breaking API change. Moving across packages does though, so we need to do that only in v4alpha via the move_to_package file-level option.

[yskopets]

@sanjaypujare We've refactored *.proto files to remove the dependency on OpenCensus.

Please give it a try when you have time.

[sanjaypujare]

@yskopets thanks a lot! When we have some time we'll go ahead and reimport these protos and verify we don't have those dependencies anymore.