Feature: Support gRPC protocol in CollectorExporter

[huandu]

Which problem is this PR solving?

In this PR, add gRPC support in CollectorExporter.

Fixes # (issue)

Short description of the change

How Has This Been Tested?

I've used this client in my production environment to export traces to Aliyun SLS, which supports gPRC protocol only.

Checklist:

• Unit tests have been added
• Documentation has been updated

[aviary2-wf]

Security Insights

No security relevant content was detected by automated scans.

Action Items

• Review PR for security impact; comment "security review required" if needed or unsure
• Verify aviary.yaml coverage of security relevant code

---

Questions or Comments? Reach out on Slack: #support-infosec.

[huandu]

This PR should be a fix for #101.

[blakeroberts-wk]

What if we avoided the switch statement using an abstract client member assigned a concrete value at initialization of this class? Something like the following:

enum CollectorExporterProtocol { http, gRPC }

abstract class _CollectorExporterClient {
  Future<void> send(pb_trace_service.ExportTraceServiceRequest body,
      Map<String, String> headers);

  void close();
}

class _HTTPExporterClient implements _CollectorExporterClient {
  static final Logger _log = Logger('opentelemetry._HTTPExporterClient');

  final Uri _uri;
  final http.Client _client;

  const _HTTPExporterClient(this._uri, this._client);

  @override
  Future<void> send(pb_trace_service.ExportTraceServiceRequest body,
      Map<String, String> headers) async {
    try {
      await _client.post(_uri,
          body: body.writeToBuffer(),
          headers: {'Content-Type': 'application/x-protobuf'}..addAll(headers));
    } catch (e) {
      _log.warning('Failed to export spans.', e);
    }
  }

  @override
  void close() {
    _client.close();
  }
}

class _GRPCExporterClient implements _CollectorExporterClient {
  static final Logger _log = Logger('opentelemetry._GRPCExporterClient');

  final dynamic _client;

  const _GRPCExporterClient(this._client);

  @override
  Future<void> send(pb_trace_service.ExportTraceServiceRequest body,
      Map<String, String> headers) async {
    try {
      await _client.export(
        body,
        options: grpc.CallOptions(
          metadata: headers,
        ),
      );
    } catch (e) {
      _log.warning('Failed to export spans.', e);
    }
  }

  @override
  void close() {}
}

class CollectorExporter implements sdk.SpanExporter {
  @Deprecated('This member will be removed in v0.19.0.')
  final Uri uri;

  @Deprecated('This member will be removed in v0.19.0.')
  final http.Client client;
  final _CollectorExporterClient _client;

  @Deprecated('This member will be removed in v0.19.0.')
  final Map<String, String> headers;
  final Map<String, String> _headers;

  late bool _isShutdown;

  CollectorExporter(this.uri,
      {http.Client? httpClient,
      this.headers = const {},
      protocol = CollectorExporterProtocol.http})
      : client = httpClient ?? http.Client(),
        _headers = headers,
        _client = protocol == CollectorExporterProtocol.http
            ? _HTTPExporterClient(uri, httpClient ?? http.Client())
            : _GRPCExporterClient(pb_trace_service_grpc.TraceServiceClient(
                grpc.ClientChannel(
                  uri.host,
                  port: uri.port,
                ),
              ));

  @override
  void export(List<sdk.ReadOnlySpan> spans) {
    if (_isShutdown) {
      return;
    }

    if (spans.isEmpty) {
      return;
    }

    final body = pb_trace_service.ExportTraceServiceRequest(
        resourceSpans: _spansToProtobuf(spans));
    _client.send(body, _headers);
  }

...

  @override
  void shutdown() {
    _isShutdown = true;
    client.close();
    _client.close();
  }
}

[huandu]

I have also considered implementing this feature with this design, but ultimately decided against it because I felt that the design was overly complex and offered a low cost-performance ratio. As shown in the code you provided above, adopting this design would require us to implement several additional client classes, which are unlikely to be reused in the current project. Considering that these clients are only capable of sending trace information and are also unlikely to be reused in the future, the practical significance of such abstraction is not substantial. Moreover, this design does not completely eliminate the need for a switch statement. As can be seen in the CollectorExporter, there is a ? : expression, which is essentially a switch statement, and it is not as clear as a switch statement. Therefore, I believe that this encapsulation does not completely eliminate switch statements but instead adds complexity.

To truly address the design issues caused by switch statements, ideally, we should use an Inversion of Control (IoC) approach to inject the clients implementing various protocols into the CollectorExporter, removing the this.uri, httpClient, and protocol parameters, and leaving the construction of the client to the caller of the library. This would best eliminate the switch statements. However, doing so would break backward compatibility with the original interface, which is a destructive upgrade. We certainly cannot make such a significant change to the interface.

Taking into account these considerations, I believe that the implementation approach in the current PR is more cost-effective than your proposal. Additionally, since the changes to the code in CollectorExporter are minimal and the logic is clearer, it would be more conducive to long-term maintenance in the future. Therefore, I hope you can reconsider this design proposal.

[blakeroberts-wk]

This makes sense. I agree with you.

It is worth note that the ternary expression in my example is done at instantiation whereas the switch statement in the current implementation is at invocation of export. With a batch processor, this creates marginal overhead. But a simple span processor might invoke export at a high frequency. Still, if this level of performance is of concern, Dart is probably not the appropriate language for the task.

I think that IoC would be an even better design. And this repository has maintained a pre-1.0 version to allow for backward incompatible changes. Of course we would need an intermediate version that maintained backward compatibility. This would most likely mean that the new implementation would need to exist as a new exporter class while the old exporter class remained. There will most likely be at least one renaming of this class prior to the 1.0 release of the software. Most likely, the renaming strategy would follow the JS implementation which uses several packages to accomplish IoC between protocol and platform: https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages.

Though I would argue supporting the feature, gRPC, in a backward compatible way with minimal changes and complexity, is valuable. So I'm on board.

[huandu]

@blakeroberts-wk The CI job test-darv2 fails because that an outdated protobuf package (2.1.0) is installed but gRPC requires at least 3.x. I update the pubspec.yaml and set protobuf: ^3.0.0.

[blakeroberts-wk]

@huandu I think we may need to make the grpc exporter a sub package so we can increase the dart sdk requirement to ^3 while still allowing ^2 for non-grpc usage 🤔

[huandu]

@blakeroberts-wk From my observation, the protoc_plugin is the root cause of this issue. Starting from 21.0.0, it depends on protobuf: ^3.0.0. Unfortunately, the last gRPC package running on dart v2 depends on protobuf: ^2.0.0. I think one possible solution to fix this issue is to install protoc_plugin 20.0.1 on dart v2. It requires some hacks in Makefile.