Add auto-configuration for OTLP gRPC format when using tracing

[timpeeters]

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.

[mhalbritter]

Thanks for the PR. I've left a comment.

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?

[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 ...

[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?

[mhalbritter]

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

[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.

[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.

[mhalbritter]

Maybe the OP can elaborate on why #35863 was created in the first place?

That would be @vpavic.

[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.

[mhalbritter]

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

[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?

[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.

[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?

[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.

[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.

[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.

[mhalbritter]

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

[timpeeters]

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

Big thanks to all of you for the excellent guidance.

[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.

[mhalbritter]

I've reverted the revert from 653443a (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 #41460.