Conversation
Signed-off-by: dpl <dpl0@users.noreply.github.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: dpl <dpl0@users.noreply.github.com>
Change SummaryAdds public |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| #lt_token #(#lifetimes_without_constraints2,)* #(#generic_types_without_constraints2),* #gt_token | ||
| #where_clause | ||
| { | ||
| fn flatten(&self, _buffer: &mut impl #veecle_os_runtime::MetricBuffer) {} |
There was a problem hiding this comment.
The derived Flatten impl is unconditionally empty, which means every type using #[derive(Storable)] silently produces zero telemetry — even when the struct has primitive fields whose own Flatten impls would emit metrics. This contradicts the trait docs at veecle-os-runtime/src/datastore/storable.rs:64-65: "Composite types should implement this trait to emit one entry per field."
If the intent is for the derive to handle the common case automatically, it should iterate over the struct's fields and emit one flatten() call per field (with the field name as the key). If the intent is that users must always write Flatten by hand for composites, the derive shouldn't emit an impl at all (otherwise it silently shadows what the user might want to write, and the orphan rules will block adding one later). The current state — derive always emits an empty impl — combines the worst of both options.
| impl_flatten_integer!(u8, u16, u32, i8, i16, i32, i64); | ||
| impl_flatten_float!(f32, f64); |
There was a problem hiding this comment.
u64, usize, and isize are missing from the integer impls. These are common Rust types — a struct with a usize length field cannot have a hand-written Flatten that defers to the field's impl, since none exists. u64 is more delicate: the Value enum only carries I64, so a blanket *self as i64 would silently truncate values ≥ 2⁶³; either skip u64 deliberately and document the omission in the trait docs, or convert via try_into() and decide what to emit on overflow.
For usize/isize either add a cast-to-i64 impl (with a doc comment about the platform-dependent range), or document why they are intentionally unsupported.
| impl<T: Flatten, const N: usize> Flatten for [T; N] { | ||
| fn flatten(&self, buffer: &mut impl MetricBuffer) { | ||
| for item in self { | ||
| item.flatten(buffer); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
For an array of primitives, every element calls add_metric("value", ...) — so an [u8; 4] produces four entries that all share the key "value", with no way for a downstream MetricBuffer to tell them apart. Either use indexed keys (e.g. "[0]", "[1]", …, which requires a buffer interface that accepts non-'static keys, or a fixed lookup table for small N), or drop this blanket impl and require users to flatten arrays explicitly so they can choose the keying scheme.
The same concern applies to the broader MetricBuffer contract: the trait docs don't say whether add_metric is expected to deduplicate keys, overwrite, or accept duplicates. Please pin this down before stabilizing the API.
| let lifetimes_without_constraints2 = self.lifetimes_without_constraints(); | ||
| let generic_types_without_constraints2 = self.generic_types_without_constraints(); |
There was a problem hiding this comment.
The …2 suffix on these bindings is unclear — the reader has to figure out from the call site that quote!'s repetition consumes the Vec, forcing a second copy. Either .clone() the original Vecs with descriptive names, or rename to something like lifetimes_for_flatten_impl / generic_types_for_flatten_impl so the intent is self-documenting.
| pub trait Flatten { | ||
| /// Writes telemetry key-value pairs into the given `buffer`. | ||
| fn flatten(&self, buffer: &mut impl MetricBuffer); | ||
| } | ||
|
|
||
| /// Receiver for key-value telemetry entries produced by [`Flatten::flatten`]. | ||
| /// | ||
| /// Implementors define how and where metric entries are stored. | ||
| pub trait MetricBuffer { | ||
| /// Adds a metric entry to this buffer. | ||
| fn add_metric( | ||
| &mut self, | ||
| key: &'static str, | ||
| value: veecle_telemetry::protocol::transient::Value<'static>, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The trait docs leave several behaviors undefined that consumers will need to know:
- Key ownership/uniqueness: keys are
&'static str, but the docs don't say whether duplicate keys within a singleflattencall are allowed or how a buffer should resolve them. - Nesting: "one entry per field" doesn't explain how nested composites should namespace their keys (e.g. should an inner struct prefix its keys with the outer field name?). Without this, two consumers will pick incompatible conventions.
- "Default implementation" in line 63 is misleading — these are blanket impls on concrete primitive types, not Rust trait default methods. Worth rewording to avoid confusion.
Additionally, no MetricBuffer implementation or non-empty Flatten impl is exercised anywhere in this PR (codecov shows 9.5% patch coverage), so the end-to-end shape of the API is unverified. Adding a small unit test with an in-memory MetricBuffer (e.g. a Vec<(&'static str, OwnedValue)>) would lock in the contract before downstream code starts depending on it.
| impl Storable for Output { | ||
| type DataType = Yoke<YokeWrapper<'static>, Chunk<'static, [u8; 84]>>; | ||
| type DataType = ParsedMessage; | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct ParsedMessage(Yoke<YokeWrapper<'static>, Chunk<'static, [u8; 84]>>); | ||
|
|
||
| impl veecle_os_runtime::Flatten for ParsedMessage { | ||
| fn flatten(&self, _buffer: &mut impl veecle_os_runtime::MetricBuffer) {} | ||
| } | ||
|
|
||
| impl core::ops::Deref for ParsedMessage { | ||
| type Target = Yoke<YokeWrapper<'static>, Chunk<'static, [u8; 84]>>; | ||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } | ||
|
|
||
| impl From<Yoke<YokeWrapper<'static>, Chunk<'static, [u8; 84]>>> for ParsedMessage { | ||
| fn from(yoke: Yoke<YokeWrapper<'static>, Chunk<'static, [u8; 84]>>) -> Self { | ||
| Self(yoke) | ||
| } |
There was a problem hiding this comment.
Adding Flatten as a supertrait bound on Storable::DataType makes it impossible to use foreign types like Yoke<...> directly as a DataType (orphan rules). Every consumer who currently stores a foreign type — this test is one example, but downstream user code will hit the same wall — has to introduce a Deref-only newtype.
Worth either (a) calling out this migration path explicitly in the Flatten/Storable docs so users know what to do when they hit the trait Flatten is not implemented for ForeignType, or (b) reconsidering whether Flatten should be a supertrait of Storable::DataType at all versus an optional extension trait used only by the telemetry path.
| /// For primitive types (e.g. `u8`, `f32`, `bool`), a default implementation is provided that | ||
| /// emits a single `"value"` key. Composite types should implement this trait to emit one entry | ||
| /// per field. Types that do not produce telemetry can provide an empty implementation. | ||
| pub trait Flatten { |
There was a problem hiding this comment.
This should be part of veecle-telemetry, it's not specific to our runtime.
EDIT: Also, not a fan of the name.
| fn add_metric( | ||
| &mut self, | ||
| key: &'static str, | ||
| value: veecle_telemetry::protocol::transient::Value<'static>, |
There was a problem hiding this comment.
'static is over-constraining here, and I don't think this MetricBuffer trait is usable without allocation when the attributes get passed as &[KeyValue] inside veecle-telemetry.
| $( | ||
| impl Flatten for $ty { | ||
| fn flatten(&self, buffer: &mut impl MetricBuffer) { | ||
| buffer.add_metric("value", veecle_telemetry::protocol::transient::Value::I64(*self as i64)); |
There was a problem hiding this comment.
i64::from(self), we don't want hidden truncation.
| } | ||
| } | ||
|
|
||
| impl<T: Flatten, const N: usize> Flatten for [T; N] { |
There was a problem hiding this comment.
I don't think we want this impl, anything more complex than primitives should be implemented manually since we can't know the semantics.
| #[automatically_derived] | ||
| impl | ||
| #lt_token #generic_params #gt_token | ||
| #veecle_os_runtime::Flatten for #ident |
There was a problem hiding this comment.
I would not derive this, we cannot know anything about the semantics here.
| pub trait Storable { | ||
| /// The data type being read/written from/to a slot. | ||
| type DataType: Debug; | ||
| type DataType: Debug + Flatten; |
There was a problem hiding this comment.
| type DataType: Debug + Flatten; | |
| type DataType: Flatten; |
We really don't want to be debug formatting on every write, that was just a hack because we had no way to pass structured values.
2 participants
I've basically added no-op implementations of the trait (not ideal, but it makes the tests pass).
Closes: DEV-1872