[OpAMP] Enhance EffectiveConfigFile: stream APIs, size limits, tests#4285
Conversation
- Add CreateFromStream and CreateFromStreamAsync to EffectiveConfigFile for safe, size-limited config loading from streams (sync/async) with enforced max byte length. - Remove CreateFromFilePath. - Content is now ReadOnlyMemory<byte>. - Enforce non-null contentType and fileName (default to empty string). - Improve OpAmpClient API docs with thrown exception details. - Add comprehensive EffectiveConfigFile tests (validation, limits, event logging). - Update FrameBuilderTests and OpAmpClientTests for new APIs.
stevejgordon
changed the title
github-actions
Bot
added
the
comp:opamp.client
| override OpenTelemetry.OpAmp.Client.Settings.AnyValueUnion.Equals(object? obj) -> bool | ||
| override OpenTelemetry.OpAmp.Client.Settings.AnyValueUnion.GetHashCode() -> int | ||
| static OpenTelemetry.OpAmp.Client.Messages.EffectiveConfigFile.CreateFromFilePath(string! filePath, string! contentType, string? filename = null) -> OpenTelemetry.OpAmp.Client.Messages.EffectiveConfigFile! | ||
| static OpenTelemetry.OpAmp.Client.Messages.EffectiveConfigFile.CreateFromStream(System.IO.Stream! stream, string! contentType = "", string! fileName = "", int maxBytes = 524288) -> OpenTelemetry.OpAmp.Client.Messages.EffectiveConfigFile! |
There was a problem hiding this comment.
Just skimming the API.
The default value attributes are bad practice from the public API perspective and long term of maintenance.
There was a problem hiding this comment.
Fair enough, I can tweak that.
There was a problem hiding this comment.
It was mostly to provide a sensible max bytes default
There was a problem hiding this comment.
An options class to hold the possible values also has the flexibility to make additions without the complexity of binary breaking changes with default/optional parameters.
There was a problem hiding this comment.
Content type and file name don't need defaults, I did that to mirror the original method null defaults which are not really valid here. On reflection, requiring those explicitly makes sense. For max size, as nothing else is likely to be needed here, I think an options type is overkill. I think either just requiring that explicitly, or having an overload without it (which then applies the default by calling the limited overload) is better. Open to opinions.
There was a problem hiding this comment.
Maybe instead of 524288, so something like StreamReader does and have -1 as a sentinel for the default size?
There was a problem hiding this comment.
Yeah, that also isn't my favourite API design for discoverability. I'm now leaning toward just requiring everything as it has a clear purpose and the ctor can be used with a pre-limited block of memory for alternative scenarios.
There was a problem hiding this comment.
For now, I've made everything required on both this factory methods and the ctor. In general, I think this is fine for this API, but I'm happy to tweak it further.
| return new EffectiveConfigFile(ReadOnlyMemory<byte>.Empty, contentType, fileName); | ||
| } | ||
|
|
||
| var buffer = ArrayPool<byte>.Shared.Rent(maxBytes); |
There was a problem hiding this comment.
This stuff looks like the code we added recently to encode limits in a shared place - can we add this code there and consume it, rather than make more copies of the pattern we need to verify?
There was a problem hiding this comment.
It's quite similar to the stuff added to HttpClientHelpers, but that code isn't quite the right fit as-is and would need generalising futher (exception messages etc) for this use case. Could investigate that as a low-priority follow-up?
Co-authored-by: Martin Costello <martin@martincostello.com>
e338ef4
4 participants
Fixes #4151
Changes
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes