Improve OtlpMetricsSender API

[shakuzen]

Remove the possible inconsistency where the sender could be given an instance of OtlpConfig that differs from the one passed to OtlpMeterRegistry. It is not clear which would be used or why. Now it is clear that the config on the registry will be used and the same sender can be used with different registry instances, potentially configured with different addresses (OtlpConfig#url).

See #5691 and specifically #5691 (comment). Breaks the API introduced in the previous milestone for #5690.

[shakuzen]

I think taking the address as a String is generic enough to work for other implementations than the HTTP one, but let me know if anyone has a concern about this. The implementation is responsible for parsing the address String. The OtlpMeterRegistry will always send the OtlpConfig#url as the address, so while it may semantically be weird if the address isn't actually a URL but the configured sender implementation can handle it (since it is a String that isn't necessarily a URL), this should work nonetheless.

[kasparkivistik]

I'm always cautious of adding extra parameters to methods. Would the API be cleaner if send method's parameters were consolidated to a value type?

Something like this (pseudocode incoming)

interface MetricsSender {

    /**
     * Send encoded metrics data from a {@link io.micrometer.core.instrument.MeterRegistry
     * MeterRegistry}.
     * @param metricsData encoded batch of metrics
     * @param headers metadata to send as headers with the metrics data
     */
    void send(MetricsSenderPayload payload);

    class MetricsSenderPayload {
        private final String address;
        private final byte[] payload;
        private final Map<String, String> headers;

        // constructor and accessors omitted
    }

and then MeterRegistry can just do this

MetricsSender.MetricsSenderPayload payload = new MetricsSender.MetricsSenderPayload(
    this.config.url(),
    request.toByteArray(),
    this.config.headers()
);
metricsSender.send(payload);

[shakuzen]

Thanks for the review. We could do something like that, and there's precedent in the codebase with HttpSender taking a Request object. I can take a closer look at it tomorrow.

[shakuzen]

I've played around with this a bit locally, but there are a number of choices, especially given the package-private MetricsSender (I would probably get rid of MetricsSender entirely when adding a Request type for simplicity). I've suggested to the team a pragmatic way of moving forward would be to merge this as is, since it fixes the issue described in the PR description. Then in a follow-up PR, probably after the milestone release set to go out today, we can iterate on the feedback given.

[jonatan-ivanov]

I'm totally fine calling it address but if you are interested in an alternative: endpoint.

[shakuzen]

I didn't mention it in the PR description but throws Throwable was added here and elsewhere. The point was to ensure that the caller (MeterRegistry publish implementation) handles the exception case, usually with logging. HttpSender similarly declares throws Throwable. That said, it may be worth looking at doing things differently here because catching Throwable includes things like OOME, as pointed out by @marcingrzejszczak in an offline review.

[jonatan-ivanov]

We can catch Exception instead.

[jonatan-ivanov]

Would be the address, headers, data order more logical?

Suggested change

	void send(String address, byte[] metricsData, Map<String, String> headers) throws Throwable;
	void send(String address, Map<String, String> headers, byte[] metricsData) throws Throwable;

[shakuzen]

I'm not sure. I think arguments could be made for a variety of orders. Switching to a builder pattern may help avoid the matter of order entirely.

[jonatan-ivanov]

We can catch Exception instead.

[jonatan-ivanov]

I'm thinking if we should make this more general and public so that other implementations can reuse it and users can get the same exception class for the different implementation.

[jonatan-ivanov]

I'm totally fine calling it address but if you are interested in an alternative: endpoint.

[shakuzen]

I've opened #6025 as a follow-up