I think usually we expect parameters to be non-null

Got it, thanks. I'll add the package annotation and remove these.

It's updated now.

is this a typo? I would expect this class to be part of the public api since it is needed to build the client.

I couldn't find where we discussed this, though I remember talking about we (the OTel Java community) not being ready to have a standalone opamp client repo and committing to provide support for it right now, so my understanding was that this contrib module would be a sort of incubating place for us to use and refine its API in the meantime without fearing of affecting users with breaking changes, and so that's why everything here is in an internal package right now.

Now, I'm fine with having a non-internal API so long as we can still add breaking changes in the future without issues, which I think it's the case for all the contrib modules in alpha status, so if that's the case, I think it's fine to move this (and other types as needed) out of the internal package, because right now, even though I remember that we wanted to use this internally for now, I don't recall having everything in an internal package to be a requirement.

I'd like to hear your thoughts on it.

I think it would be better to move the api out of the internal package as then we could get a better feel of what apis we have and what classes they need. We'll have to do this eventually anyway. If needed we can make parts of the api experimental (or should I say more experimental than the rest) and have them in the internal package. In the agent repository we have done this for example in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/Experimental.java @trask do you have any suggestions

It could depend on whether we're planning to publish a general purpose OpAmp client library, or whether this is eventually only going to be an implementation detail of a higher level OpAmp-based remote configuration solution.

I don't mind either approach for now.

I'm fine with either approach as long as it's ok to add breaking changes without worrying about affecting users, as my understanding is that this module should serve as an incubator to iteratively define the client API that better works for Java. In any case, since this PR is approved, and adding this change might make the PR quite bigger, I've created this issue to keep track of this discussion.

should this class be in the internal package?

This reply applies here too.

perhaps it would make sense to default to http?

I've added http as the default. My only concern is that it ties the builder with the okhttpsender impl, though that's not a problem for now.

Yes, this will definitely require some more thought. I guess the whole point in having a custom HttpSender would be to use something else besides okhttp. From that perspective all of the okhttp specific code should be in a separate module. On the other hand most users are going to be fine with okhttp and perhaps only wish to provide the url, for them it would be nice to have s simpler option than builder.setRequestService(HttpRequestService.create(OkHttpSender.create("http://localhost:4320/v1/opamp"))). We could build a spi similar to HttpSenderProvider in the sdk that could default to okhttp when it is available and when some other provider is available use that. The tricky part would be when there are multiple providers. Sdk resolves that through configuration options, but that probably is not going to work here. Anyway this is not for the current PR.

could add a package-info with @ParametersAreNonnullByDefault into this package too

we typically only annotate @nullable params, see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/style-guideline.md#nullable-annotation-usage

Thanks. I've just added the changes. I was confused at first because I thought that the package-info file was also applied to the subpackages automatically, though the IDE kept complaining until I added it to this specific one.

why do the callbacks pass in OpampClient? it seems that is available if needed already when registering the callback?

This is a good point, and tbh I don't have a good answer for it 😅 I just saw that in the opamp-go callback they pass a Context as first param, which is a go-specific type, and it just felt like it might be helpful to pass the client impl here instead, as a way to provide some sort of context of where was this callback called from, in case that there might be multiple clients using a single callback? It's a bit of a stretch, and tbh, after thinking about it again now, I couldn't find a use case for it, so I just removed it following the "when in doubt, leave it out" mantra. In any case, since this is an incutating API, we could add it back later if needed.