Refactor http, grpc senders and promote to public API

[jack-berg]

Resolves #6718.

Promote the HttpSender, GrpcSender interfaces to the public API. There's a lot. Summary:

• We need to hide Marshaler and related APIs, since they balloon the API surface area and would take lot of work to get ready for public. Introducing narrow focused MessageWriter to serve the function currently performed by Marshaler.
• Need to get rid of MarshalerServiceStub and related APIs from the gRPC senders stuff. It drags in a bunch of unnecessary cruft and io.grpc.stub dependency.
• Need to hide the HttpSenderConfig#getExportAsJson() option. Senders don't need to be burdened with understanding whether the request is binary or JSON. They just need to be told the content type and a way to write the request body.
• Compressor and related APIs need to get promoted as well.
• Need to refine GrpcSenderConfig, HttpSenderConfig
  • Need to be interfaces instead of autovalue classes
  • Need javadoc
  • Make endpoint URI instead of string
  • Provide grpc senders the fully qualified service name and method name
  • Hide the Object GrpcSenderConfig#getManagedChannel(), which is required for backwards compatibility and for UpstreamGrpcSender, behind an internal-only ExtendedGrpcSenderConfig

[brunobat]

@jack-berg, I didn't forgot this issue. I'm planning to give you a response in the week of Nov. 17.

[brunobat]

@jack-berg, thanks very much for this.
The end user changes are fair enough and after updating the Quarkus code, it worked out of the box and the IT tests with a real OTel Collector pass.
I'm adding a couple of comments.

[brunobat]

I've taken a look at the declarative config being proposed. We currently don't use it but I have a few comments.

[zeitlinger]

The end user changes are fair enough and after updating the Quarkus code

Why is this relavant for Quarkus? I'm asking because the spring starter doesn't care - and I'm trying to understand what the difference is

[jack-berg]

I've taken a look at the declarative config being proposed.

There's actually nothing in here related to declarative config. The getters in HttpSenderConfig, GrpcSenderConfig reflect the standard configuration options we expose via programmatic config for OTLP exporters (e.g. OtlpHttpSpanExporterBuilder, OtlpGrpcSpanExporterBuilder).

As the set of configuration options we expose via the Otlp{Signal}{Protocol}ExporterBuilders evolves, HttpSenderConfig and GrpcSenderConfig will evolve along side.

I would say that if a sender ignores any of those options, its technically not compliant. Non-compliant doesn't mean its not useful, but it does mean that the sender isn't able to behave like the user expects based on how they configured the builder.

[brunobat]

The end user changes are fair enough and after updating the Quarkus code

Why is this relavant for Quarkus? I'm asking because the spring starter doesn't care - and I'm trying to understand what the difference is

Because Quarkus has it's own senders based on a Vert.x client. It does not use OkHttp. See #6718 and its comments for more details.

[brunobat]

I've taken a look at the declarative config being proposed.

There's actually nothing in here related to declarative config. The getters in HttpSenderConfig, GrpcSenderConfig reflect the standard configuration options we expose via programmatic config for OTLP exporters (e.g. OtlpHttpSpanExporterBuilder, OtlpGrpcSpanExporterBuilder).

As the set of configuration options we expose via the Otlp{Signal}{Protocol}ExporterBuilders evolves, HttpSenderConfig and GrpcSenderConfig will evolve along side.

I would say that if a sender ignores any of those options, its technically not compliant. Non-compliant doesn't mean its not useful, but it does mean that the sender isn't able to behave like the user expects based on how they configured the builder.

Thanks for the explanation @jack-berg.
If those properties are not used, the compliance is left to the implementator.
As in the other properties, we always try to match the property behavior, even if properties are handled by Quarkus and some behavior is not implemented directly on the SDK. We have other examples with samplers and several customizations we provide for the SDK.
The Idea is to melt the OTel experience into Quarkus and have minimal friction for people with knowledge of both frameworks.

[jack-berg]

Ok so it seems like something close to this will work for quarkus. There are still some small tweaks (example), but its mostly correct.

The next step will be to coordinate with @jkwatson and @open-telemetry/java-approvers to choose a strategy and schedule to get this merged. I opened this as one big PR to make the whole picture clear, but I'm happy to break it up in smaller more reviewable pieces as long as we can commit to work quickly to get all those pieces merged for a single release. This will be important to minimize churn.

[trask]

(got through the easy 75/94 files in this first round)

[brunobat]

Hi @jack-berg Any news about the split of this work into several PRs?

[trask]

I agreed to review this one big PR. Once @jack-berg addresses the first round of comments I'll make the next pass.

[jack-berg]

Thanks for the poke @trask - will clean this up, incorporate your first round of fedback, and mark it ready for review.

[codecov]

Codecov Report

❌ Patch coverage is 85.09804% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (62049e7) to head (a392cd0).

Files with missing lines	Patch %	Lines
...orter/sender/okhttp/internal/OkHttpGrpcSender.java	60.86%	8 Missing and 1 partial ⚠️
...ension/trace/jaeger/sampler/OkHttpGrpcService.java	54.54%	3 Missing and 2 partials ⚠️
...telemetry/exporter/internal/marshal/Marshaler.java	50.00%	4 Missing ⚠️
...pc/managedchannel/internal/UpstreamGrpcSender.java	91.11%	2 Missing and 2 partials ⚠️
...edchannel/internal/UpstreamGrpcSenderProvider.java	71.42%	2 Missing and 2 partials ⚠️
...y/exporter/internal/grpc/MarshalerInputStream.java	57.14%	3 Missing ⚠️
...orter/sender/okhttp/internal/OkHttpHttpSender.java	85.00%	3 Missing ⚠️
...io/opentelemetry/exporter/grpc/GrpcStatusCode.java	92.30%	1 Missing and 1 partial ⚠️
...a/io/opentelemetry/exporter/grpc/GrpcResponse.java	0.00%	1 Missing ⚠️
...a/io/opentelemetry/exporter/http/HttpResponse.java	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7782      +/-   ##
============================================
- Coverage     90.15%   90.12%   -0.04%     
+ Complexity     7476     7468       -8     
============================================
  Files           836      835       -1     
  Lines         22550    22525      -25     
  Branches       2224     2230       +6     
============================================
- Hits          20331    20300      -31     
- Misses         1515     1519       +4     
- Partials        704      706       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jack-berg]

Ready for review. Updated the description to match the current state of things.

[trask]

(just the next round)

[trask]

I don't think the / should be needed

Suggested change

	.resolve("/" + grpcSenderConfig.getServiceAndMethodName())
	.resolve(grpcSenderConfig.getServiceAndMethodName())

[jack-berg]

The build was falling without it! Couldn't recreate it locally. Need to dig into it more.

[jack-berg]

The build was falling without it! Couldn't recreate it locally. Need to dig into it more.

[jack-berg]

Yeah there's a change in behavior in URI.resolve from java 11 to java 17. See this PR which reduces the change to its essence, passing on java 17+, failing on java 11 and lower: #7969

Oddly, I can't seem to find anything on the internet acknowledging this change in behavior.. But look at the build, its there!

[trask]

wow 😱

[jack-berg]

Was thinking about this last night and was suddenly worried we were publishing these interfaces in the wrong artifact.

As proposed currently, the new interfaces are published to opentelemetry-exporter-common in the package io.opentelemetry.exporter.common.

This is suboptimal because opentelemetry-exporter-common contains marshaler utilities in an internal package, which senders aren't concerned with.

Other possible homes include:

• opentelemetry-exporter-otlp-common: If sender concept only applies to OTLP exporter (currently the case) then arguably the OTLP comon package should be the home. But OTLP common contains all the hand rolled marshalers, which senders aren't concerned with.
• opentelemetry-exporter-sender-api: What if we introduce a brand new artifact with only the sender APIs and nothing else? Unfortunately, we / I made the design decision a while back to put RetryPolicy in opentelemetry-sdk-common. And then we doubled down by adding ProxyOptions and MemoryMode to SDK common. So a dedicated sender API artifact still needs to depend on opentelemetry-sdk-common, and we have yet another artifact.
• opentelemetry-sdk-common: This is the conclusion I reached, balancing the ideal of having a minimal artifact for senders to depend on with the reality of where RetryPolicy, ProxyOptions, Memory mode are currently packaged. We add all the newly public sender APIs to opentelemetry-sdk-common in the io.opentelemetry.sdk.common.export package. The only downside of this is that senders bring in irrelevant SDK utility classes like Resource, Clock, InstrumentationScopeInfo, etc. But its better than needing to depend on the marsheler utils of opentelemetry-exporter-common. We could of course deprecate / move the RetryPolicy, ProxyOptions, MemoryMode to a more appropriate home, but I don't think this is a big enough deal to warrant the churn.

What do you think @trask / @open-telemetry/java-approvers? I have a commit read to move the API to opentelemetry-sdk-common, but will wait for the discussion. Also, I figure we can get through the PR review as is, and then make one final commit to move to the final home, which would be a strict refactor and trivial to review.

[trask]

I figure we can get through the PR review as is, and then make one final commit to move to the final home, which would be a strict refactor and trivial to review

👍