This pull request introduces opt-in auto-configuration for OtlpGrpcSpanExporter (fixes #35863). The implementation remains backwards compatible and still configures the existing OtlpHttpSpanExporter by default. Using a new configuration property, it is possible to change the transport: management.otlp.tracing.transport.

Comment From: timpeeters

Just an idea I had, but I'm not sure if this is common: we could use the URL to determine the kind of transport to use. http:// or https:// would use OtlpHttpSpanExporter, grpc:// or grpcs:// (?) would use OtlpGrpcSpanExporter.

What do you think?

I was actually considering something along the same line. I thought about using the port number to deduct the transport to use: 4137 for grpc and 4318 for http (otel defaults). However I believe the end-user should still be able to override this auto-detect in case they are using non-default ports. Would an "auto-detect" option in the enum be acceptable here?

I haven't seen people use grpc:// or grpcs:// so far. But I'll do some research ...

Comment From: timpeeters

I haven't seen people use grpc:// or grpcs:// so far. But I'll do some research ...

Indeed, grpc:// or grpcs:// does not really seem to be a thing. How do you propose we proceed here?

Comment From: mhalbritter

If that's not a thing, i think we shouldn't invent it and go with the transport property.

Comment From: jonatan-ivanov

A bit aside of these changes: why do you want to use gRPC?

There are three transports in the OTLP specs (http/protobuf, http/json, grpc). It is also specified that every collector/backend must support all three transports so that clients (Spring Boot apps) don't need to (it does not matter what transport your client is using from the collector's perspective). Also, last time I had discussions about this with OTel folks, gRPC did not seem to be faster or more efficient than http/protobuf, gRPC is using HTTP2 under the hood and both support the same (compressed) protobuf format. There is one difference though: people run into issues with gRPC if they have another gRPC client in their apps.

More connected to this PR: I think integration tests are missing (see: OtlpAutoConfigurationIntegrationTests), they may not be easy to add for gRPC.

Comment From: timpeeters

A bit aside of these changes: why do you want to use gRPC?

gRPC is already enabled for the ingestion of traces coming from the OpenTelemetry integration in ingress-nginx. From an infra point of view I can understand the desire to limit the amount of open ports. Given that the gRPC exporter is already on the classpath and #35863 was there, I assumed it would be a good candidate to add. Maybe the OP can elaborate on why #35863 was created in the first place?

More connected to this PR: I think integration tests are missing (see: OtlpAutoConfigurationIntegrationTests), they could also not be easy to add for gRPC.

I'll look into the possibilities here.

Comment From: mhalbritter

Maybe the OP can elaborate on why https://github.com/spring-projects/spring-boot/issues/35863 was created in the first place?

That would be @vpavic.

Comment From: mhalbritter

Regarding the URL scheme:

grpc itself uses this scheme. I haven't tested what OtlpGrpcSpanExporter accepts as an URL, but I guess this is passed as is to the grpc client underneath. If we would limit that to grpc:// urls, things like dns:///foo.googleapis.com:8080 can't be used. I think I'm leaning towards passing the url as is and add a new transport property, like this PR does right now.

Comment From: mhalbritter

Okay, nevermind, Otel calls io.opentelemetry.exporter.internal.ExporterBuilderUtil#validateEndpoint, which only accepts http:// and https://.

Comment From: timpeeters

I fiddled around a bit but I'm not yet convinced I'm on the right track for an integration tests for the OtlpGrpcSpanExporter.

The following code:

private final Server s = ServerBuilder.forPort(0)
        .addService(new GrpcService())
        .build();

private static final class MockGrpcService extends TraceServiceGrpc.TraceServiceImplBase {

    private ExportTraceServiceRequest lastRequest;

    @Override
    public void export(ExportTraceServiceRequest request,
            StreamObserver<ExportTraceServiceResponse> responseObserver) {
        this.lastRequest = request;
        responseObserver.onNext(ExportTraceServiceResponse.getDefaultInstance());
        responseObserver.onCompleted();
    }

    ExportTraceServiceRequest getLastRequest() {
        return this.lastRequest;
    }
}

Allows me to capture gRPC requests, at the cost of running an additional Server and an additional dependency: io.grpc:grpc-testing:1.62.2. For which there is no dependency management in place yet. This all feels a bit clunky. Any ideas how to proceed @jonatan-ivanov?

Comment From: jonatan-ivanov

@mhalbritter

If we would limit that to grpc:// urls, things like dns:///foo.googleapis.com:8080 can't be used. I think I'm leaning towards passing the url as is and add a new transport property, like this PR does right now.

I think the transport property might be the safer bet but having multiple things in the scheme/protocol is valid, see JDBC urls:

jdbc:postgresql://localhost:5430/db
jdbc:mysql://localhost:3306/db

Similarly, the url in the Spring property can be something like:

grpc:dns:///foo.googleapis.com:8080
grpc:dns:foo.googleapis.com:8080

I don't know/think that the OTel client will work with it so it would be on Boot to remove the grpc: prefix from the url.

@timpeeters

Allows me to capture gRPC requests, at the cost of running an additional Server and an additional dependency: io.grpc:grpc-testing:1.62.2. For which there is no dependency management in place yet. This all feels a bit clunky. Any ideas how to proceed @jonatan-ivanov?

I think running a server is the point of having integration tests, I consider it as a feature in this case. On the other hand, since gRPC is "just" HTTP2, MockWebServer (see current tests) might be able to receive the payload with a bit of luck, the question is if they do what we can do to verify it (it's binary, we need to deserialize). I remember playing with this earlier but I don't remember how far I got. I think MockGrpcService could work (instead of last I would use a queue though) but using the dependency and the mock server is up to the Boot team to decide.

Comment From: timpeeters

I added a new commit with a first attempt for an integration test. Looking forward to some feedback.

Initially I tried to use the existing MockWebServer but it is unable to parse the request and I could not find a way to make it work with gRPC requests.

java.lang.StringIndexOutOfBoundsException: begin 0, end -1, length 2
    at java.base/java.lang.String.checkBoundsBeginEnd(String.java:4606)
    at java.base/java.lang.String.substring(String.java:2709)
    at okhttp3.mockwebserver.RecordedRequest.<init>(RecordedRequest.kt:99)
    at okhttp3.mockwebserver.MockWebServer.readRequest(MockWebServer.kt:751)

Using io.grpc.Server instead, I'm able to get it to work, however at the cost of the additional test scoped dependency (which btw is io.opentelemetry.proto:opentelemetry-proto:1.2.0-alpha and not io.grpc:grpc-testing:1.62.2 as I mentioned before, sorry for the confusion). This dependency allows me to decode the binary request.

I'm fairly sure we can't leave a dependency like this (including the version) in the build.gradle file of spring-boot-actuator-autoconfigure. Any ideas how to move on from here?

Comment From: jonatan-ivanov

I'm fairly sure we can't leave a dependency like this (including the version) in the build.gradle file of spring-boot-actuator-autoconfigure. Any ideas how to move on from here?

Versions are defined in spring-boot-dependencies/build.gradle. Either the opentelemetry-proto or the opentelemetry-bom-alpha version can be defined there but the Boot team should make that call. Fyi: opentelemetry-proto is not marked stable yet since the profiling section is not ready (not used here): https://github.com/open-telemetry/opentelemetry-proto?tab=readme-ov-file#maturity-level.

Comment From: wilkinsona

If it's a dependency that's only used in tests, it should go in spring-boot-parent so that we're only managing the dependency for Boot's own build and not for users who consume spring-boot-dependencies.

Comment From: timpeeters

I've replaced the solution that relied on the gRPC server with a simple Jetty server. I'm no longer parsing the request to gRPC classes. I believe this matches more closely to what @jonatan-ivanov suggested above. I'm still able to verify that a request is received by the Jetty server and I'm able to apply similar assertions: headers (content-type and custom), but also the request body (as a string).

This also saves us from having to add dependency management for opentelemetry-proto which is not even included in the opentelemetry-bom or opentelemetry-bom-alpha.

Now it looks more like what was already there for the http exporter. Happy to hear what you think.

Comment From: mhalbritter

We talked about this yesterday and we don't fiddle with the url scheme. We use http:// and https:// and use a property to distinguish between http and gRCP.

Comment From: mhalbritter

Thank you very much and congratulations on your first contribution :tada:!

Comment From: timpeeters

Thank you very much and congratulations on your first contribution 🎉!

Big thanks to all of you for the excellent guidance.

Comment From: wilkinsona

Unfortunately, we've decided to revert this, hopefully temporarily. Looking at #41324 and #41333 we're not 100% sure that things are heading in the right direction.

With this change in place, we're in an inconsistent situation where you can use gRPC to export traces but not logs or metrics. We could add support for exporting logs using gRPC but support for metrics would require a change in Micrometer to its OtlpMeterRegistry. It has been requested in the past but didn't go anywhere.

If we decide to proceed with gRPC support, we'd like to give ourselves a bit of breathing room to review things for consistency. For example, it would probably make sense to move the Transport enum into a package that can be used by both the tracing and logging support. We may also want to review some of the other packages that are involved to ensure that they're named consistently.

Comment From: mhalbritter

I've reverted the revert from 653443adc1e42a6ab32a88b886cf63f56c4db3fc (which means this PR is now in main) and polished a bit: moved the Compression and Transport enum into a shared package. More refactorings are going to happen in the context of https://github.com/spring-projects/spring-boot/issues/41460.