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 88.48485% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (62049e7) to head (2e6c486).
⚠️ Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
...orter/sender/okhttp/internal/OkHttpGrpcSender.java	64.00%	8 Missing and 1 partial ⚠️
...pc/managedchannel/internal/UpstreamGrpcSender.java	89.36%	2 Missing and 3 partials ⚠️
...ension/trace/jaeger/sampler/OkHttpGrpcService.java	54.54%	3 Missing and 2 partials ⚠️
...telemetry/exporter/internal/marshal/Marshaler.java	50.00%	4 Missing ⚠️
...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	86.36%	3 Missing ⚠️
...pentelemetry/sdk/common/export/GrpcStatusCode.java	92.30%	1 Missing and 1 partial ⚠️
...telemetry/exporter/internal/http/HttpExporter.java	83.33%	0 Missing and 1 partial ⚠️
...rter/internal/metrics/ExporterInstrumentation.java	93.33%	0 Missing and 1 partial ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7782      +/-   ##
============================================
- Coverage     90.15%   90.13%   -0.03%     
+ Complexity     7476     7471       -5     
============================================
  Files           836      833       -3     
  Lines         22550    22522      -28     
  Branches       2224     2234      +10     
============================================
- Hits          20331    20300      -31     
- Misses         1515     1517       +2     
- Partials        704      705       +1     

☔ 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)

[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

👍

[trask]

lgtm

[brunobat]

Retested my Quarkus branch after the changes and still looks good.

[jack-berg]

Thank you @brunobat!

[jack-berg]

Pushed one last commit to restore support for the old io.opentelemetry.exporter.internal.http.HttpSenderProvider / io.opentelemetry.exporter.internal.grpc.GrpcSenderProvider properties for specifying sender provider, and logging a removal warning.

Planning on merging this soon and opening follow up PRs to resolve this comment: #7782 (comment)

[trask]

opening follow up PRs to resolve this comment: #7782 (comment)

I don't mind if you prefer to do that in this PR, I can just review the individual commit(s) and re-approve, whatever you prefer

[jack-berg]

Hmm.. ok let me open PRs in my fork against my public-sender-api branch. That will allow us to compare the different options side by side, while still retaining the ability to merge all of the related changes in one go. That is, after we select the option we prefer, merge those changes into this public-sender-api branch, and merge this branch into main.

Ok so here it goes:

1. Move sender to opentelemetry-sdk-common: Move sender to opentelemetry-sdk-common jack-berg/opentelemetry-java#20
2. Move sender to new opentelemetry-exporter-sender-api artifact: Move sender API to new opentelemetry-exporter-sender-api artifact jack-berg/opentelemetry-java#21
3. Consolidate sender into single package in opentelemetry-exporter-common: Consolidate sender API into single package jack-berg/opentelemetry-java#22

I don't think option 2 is any good. See reasons in PR description.

In my original comment I discussed moving to opentelemetry-exporter-otlp-common. The idea here is that the sender is only for OTLP exporters - not a generic exporter utility - so let's reflect this in the API. This is no good because lots of the utilities in opentelemetry-exporter-common depend on the sender artifacts, and opentelemetry-exporter-otlp-common depends on opentelemetry-exporter-common. This means that to avoid cyclic dependencies, all those opentelemetry-exporter-common utilities would also need to move to opentelemetry-exporter-otlp-common. This becomes a horrible mess really fast.

That leaves option 1 or 3.

The only advantage of option 3 is that it keeps opentelemetry-sdk-common smaller for users who want to use the SDK and don't want to use OTLP exporters. This is a narrow cohort. And this benefit comes at the expense of consistency, since its odd to have things like RetryPolicy, ProxyOptions, CompletableResultCodein a separate package thanCompressor`.

I suppose we could move Compressor to opentelemetry-sdk-common and keep the rest of the sender API in opentelemetry-exporter-common. I recommend doing this or option 1.

[trask]

opentelemetry-sdk-common makes sense to me. In theory the new interfaces could be used beyond just exporters, e.g. by opamp in the future?

I didn't understand this comment above though:

The only downside of this is that senders bring in irrelevant SDK utility classes like Resource, Clock, InstrumentationScopeInfo, etc.

[brunobat]

Consolidating things into io.opentelemetry.sdk.common.export looks good to me. Seems cleaner.

[jack-berg]

I didn't understand this comment above though:

The only downside of this is that senders bring in irrelevant SDK utility classes like Resource, Clock, InstrumentationScopeInfo, etc.

My point there is that in an ideal world a sender implementation can implement the API without needing to bring in unrelated dependencies. But thinking about it more, senders are only used in the context of the SDK. So while those classes aren't strictly necessary for a sender, they will always end up on the classpath of any application using a sender anyway.

[jack-berg]

Ok I went ahead and updated this PR branch to move the sender API to opentelemetry-sdk-common in the io.opentelemetry.sdk.common.export package.

Will give it a day or so for any additional comments and then go ahead and merge. Thanks for all the effort spent reviewing this @trask / @brunobat!

[trask]

In theory the new interfaces could be used beyond just exporters, e.g. by opamp in the future?

Or for jaeger remote sampler?

@jack-berg is this correct? if so, maybe io.opentelemetry.sdk.common.sender instead of io.opentelemetry.sdk.common.export

[jack-berg]

@jack-berg is this correct?

Yes its correct in theory. Though in practice, jaeger remote sampler would need a significant refactor to be built on top of the sender API.

if so, maybe io.opentelemetry.sdk.common.sender instead of io.opentelemetry.sdk.common.export

RetryPolicy and ProxyOptions are already in io.opentelemetry.sdk.common.export, and these are also could be useful to jaeger remote sampler. We might think of io.opentelemetry.sdk.common.export as a collection of utilities for building components that require network connections. Typically this is exporters, but might also include samplers and potentially other components.

[trask]

RetryPolicy and ProxyOptions are already in io.opentelemetry.sdk.common.export

ah, I missed that, 👍

[brunobat]

Thanks very much @jack-berg