[OpAMP] Expose public RemoteConfigMessage

[stevejgordon]

Fixes #3613

Changes

• Expose RemoteConfigMessage on the public API
• Add supporting types for remote config
• Update unshipped public API

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.47%. Comparing base (f441e6e) to head (727de3e).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3614      +/-   ##
==========================================
+ Coverage   71.43%   71.47%   +0.04%     
==========================================
  Files         442      443       +1     
  Lines       17522    17535      +13     
==========================================
+ Hits        12517    12534      +17     
+ Misses       5005     5001       -4     

Flag	Coverage Δ
unittests-OpAmp.Client	79.02% <100.00%> (+0.97%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...nt/Messages/RemoteConfiguration/AgentConfigFile.cs	100.00% <100.00%> (ø)
...essages/RemoteConfiguration/RemoteConfigMessage.cs	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[stevejgordon]

A few things for discussion:

This creates new public types for the properties of the message. Therefore this is some overhead in copying from the internal proto types. I think this is to be expected and fairly unavoidable.

This currently ignore CA1819 and exposes the raw bytes for the Body of the config. That feels like it makes sense as the consumer needs the bytes in order to handle the config in whatever format they expect. We could store the bytes in a field and consider exposing ReadonlySpan<byte> though.

/// <summary>
/// Represents an agent configuration file.
/// </summary>
public class AgentConfigFile
{
    private readonly byte[] bodyBytes;

    internal AgentConfigFile(global::OpAmp.Proto.V1.AgentConfigFile agentConfigFile)
    {
        this.bodyBytes = agentConfigFile.Body?.ToByteArray() ?? [];
        this.ContentType = agentConfigFile.ContentType;
    }

    /// <summary>
    /// Gets the raw bytes of the configuration file.
    /// </summary>
    public ReadOnlySpan<byte> Body => this.bodyBytes;

    /// <summary>
    /// Gets the MIME Content-Type that describes the data contained in the <see cref="Body"/>".
    /// </summary>
    public string? ContentType { get; }
}

At least for AgentConfigFile I left it unsealed as a consumer could create their own derived type that handles any deserialising and exposes their specific properties etc. We might want to seal other types, which I tend to do until there's an obvious case to unseal, but I'm not sure what we prefer in this repo overall.

At the moment, I've focused on making these ready only to support listeners for sever to agent messages. We may later want to make some properties settable for agent to server use cases.

[martincostello]

Are there any benefits to using an interface type if this is essentially a DTO, rather than requiring a concrete type?

Then we wouldn't need to add the suppression and if we opened up the setter people could use other types and we could handle converting it to the type needed for protobuf under the hood.

[stevejgordon]

Again, I didn't give this detail too much thought but I'm not sure if that overcomplicates things when the data is really just some bytes. It's kind of a low-level API due to the design of the remote config feature. In our Elastic use case for example, we'll have some bytes representing JSON the specifies our central config options. We'd therefore check the content type is application/json and pass the bytes into STJ to deserialise them into our type representing the configuration payload. I think most other use cases would be the same.

[stevejgordon]

We could use ICollection<byte> here, or as per #3614 (comment), we could just expose the ROS.

[stevejgordon]

@martincostello I think my main question for now, is whether this approach to exposing the public message types seems generally reasonable in this general form. We can then shape the API for each type we need for message properties.

We can then either, do a big bang to make all server-to-agent messages public, or do each one in a separate PR. I'd be quite keen to get remote config in sooner rather than later so we can experiement with using OpAMP for central config. That might identify other refinements we should do, before we focus on all message types.

[martincostello]

It makes sense to me to separate the public API from the protobuf stuff, as that's essentially the same idea as not making EFCore data types public either. Otherwise we're effectively making the generated code public API, which is probably a recipe for long-term sadness.

[stevejgordon]

@martincostello I've pushed a refined public API which I think is a nicer public API. AgentConfigFile now exposes a method to get the body bytes as well as a higher-performance API by exposing TryGetBody. There's no longer a specific dictionary type and instead RemoteConfigMessage just exposes IReadOnlyDictionary<string, AgentConfigFile>. I've made the comparer Ordinal which I think is reasonable. The spec doesn't specify anything around file name case sensitivity.

One consideration is testability for users when subscribing the messages. Right now, the ctor is internal and builds the message based on the protobuf types. We could consider providing an interface for each message to ease testability of subscribers? A risk is if the API expands, we end up modifying the interface and breaking those tests. Another option is designing this so the exposed properties can be set, but that's not straightforward for AgentConfigFile as we store the ByteString to back the methods and some properties. I don't really like exposing these publically as settable just for testing. Open to thoughts on this scenario.

If this looks good in principle, I can add some tests.

[martincostello]

LGTM so far - just a few inline thoughts.

[martincostello]

I wonder if byte[]? would be better to be able to distinguish between "not set" and "genuinely empty" (assuming such a distinction is meaningful here).

[stevejgordon]

It's unclear to me from the spec if this could ever be null. In theory, not as it should only send the config if there's something to send. My gut is that the expected scenario is there is always some data so avoiding making this nullable and requiring the user to null check is slightly more pleasant. The spec specifically calls out that the key may be empty, but doesn't specify scenarios were the config could be null.

[Kielek]

One topic to the public contract, I have missed in previous PR.

Shouldn't ReadOnlySpan<byte> be a better option here? the body, exposes Span property. We could avoid allocations.

It is related to other places in the public API

[stevejgordon]

@Kielek We did have that exposed at one point but I think at the time I'd missed that ByteString has a span directly accessible. I'll update as we can get rid of the methods and TryGet pattern, just relying on exposing the Span for consumers to do whatever they need with.

[stevejgordon]

@martincostello A few changes as per your suggestion. I've also exposed the config hash in the same way as for the body bytes. I've added unit test coverage also. I've marked ready for review and scoped this PR to specifically this message type. I've also raised #3617 to expose a way to confiure the client to identify that the client accepts this message which I'll tackle in a subsequent PR.

[stevejgordon]

@martincostello and @Kielek - The latest commit massively simplifies the public API and just exposes the ROS, letting the user use that as neccesary when their subscriber(s) handle the message.

[Kielek]

LGTMRS