feat: rest of impact metrics definitions for dotnet#76
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the remaining impact-metrics FlatBuffers messages + FFI plumbing so the .NET engine can define and update counters, gauges, and histograms via the Rust FlatBuffer FFI layer.
Changes:
- Added new Rust FlatBuffer FFI endpoints: inc counter, define/set gauge, define/observe histogram (with optional labels/buckets).
- Extended the FlatBuffers schema and regenerated Rust/C# messaging types for the new metric messages.
- Added .NET engine wrappers and tests for the new impact-metrics APIs.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
yggdrasilffi/src/flat/mod.rs |
Adds FlatBuffer FFI handlers for counter/gauge/histogram metric operations and label parsing. |
yggdrasilffi/src/flat/enabled-message_generated.rs |
Regenerated Rust FlatBuffers types for new metric messages. |
flat-buffer-defs/enabled-message.fbs |
Extends schema with IncCounter, DefineGauge, SetGauge, DefineHistogram, ObserveHistogram. |
dotnet-engine/Yggdrasil.Engine/yggdrasil/messaging/*.cs |
New autogenerated C# FlatBuffers message types for the added schema tables. |
dotnet-engine/Yggdrasil.Engine/YggdrasilEngine.cs |
Public .NET API surface for the new impact-metrics operations. |
dotnet-engine/Yggdrasil.Engine/Flatbuffers.cs |
Builders for the new FlatBuffer request payloads (incl. labels/buckets). |
dotnet-engine/Yggdrasil.Engine/Flat.cs |
Loads new native symbols and exposes managed wrappers for them. |
dotnet-engine/Yggdrasil.Engine.Tests/YggdrasilImpactMetricsTest.cs |
Adds .NET tests covering the new metric methods and basic invalid-arg behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static VectorOffset? CreateSampleLabelsVector(FlatBufferBuilder builder, IDictionary<string, string>? labels) | ||
| { | ||
| if (labels == null || labels.Count == 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var labelEntries = new Offset<SampleLabelEntry>[labels.Count]; | ||
| for (var i = 0; i < labels.Count; i++) | ||
| { | ||
| var kvp = labels.ElementAt(i); | ||
| labelEntries[i] = SampleLabelEntry.CreateSampleLabelEntry( | ||
| builder, | ||
| builder.CreateString(kvp.Key), | ||
| builder.CreateString(kvp.Value) | ||
| ); | ||
| } | ||
|
|
||
| return IncCounter.CreateLabelsVector(builder, labelEntries); |
There was a problem hiding this comment.
SampleLabelEntry.key is marked as a FlatBuffers (key) field (see enabled-message.fbs), which means vectors of SampleLabelEntry are expected to be sorted by key for generated LabelsByKey(...) / __lookup_by_key helpers to work correctly. CreateSampleLabelsVector currently preserves the IDictionary enumeration order, so any by-key lookups on these vectors may return incorrect results. Sort the label entries by key (or use the FlatBuffers helper for creating sorted vectors of tables) before creating the vector.
| ); | ||
| } | ||
|
|
||
| return IncCounter.CreateLabelsVector(builder, labelEntries); |
There was a problem hiding this comment.
CreateSampleLabelsVector returns IncCounter.CreateLabelsVector(...), which unnecessarily couples this generic label-builder to the IncCounter schema/type. This makes the code harder to maintain if IncCounter’s generated API changes, and it’s confusing when the same helper is used for SetGauge/ObserveHistogram labels. Prefer building the vector directly via the FlatBufferBuilder, or use the CreateLabelsVector method on the specific message type being built.
| return IncCounter.CreateLabelsVector(builder, labelEntries); | |
| return builder.CreateVectorOfTables(labelEntries); |
| pub unsafe extern "C" fn flat_inc_counter( | ||
| engine_ptr: *mut c_void, | ||
| message_ptr: u64, | ||
| message_len: u64, | ||
| ) -> Buf { |
There was a problem hiding this comment.
New FlatBuffer FFI endpoints for impact metrics (inc counter / gauge / histogram) are added here, but the existing test module in this file doesn’t exercise any of them. Adding a few Rust-side tests (similar to the existing flat_check_enabled / flat_take_state tests) would help catch schema/ABI mismatches early, especially around label parsing and optional bucket handling.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| private static VectorOffset? CreateSampleLabelsVector(FlatBufferBuilder builder, IDictionary<string, string>? labels) | ||
| { | ||
| if (labels == null || labels.Count == 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var labelEntries = new Offset<SampleLabelEntry>[labels.Count]; | ||
| var index = 0; | ||
| foreach (var kvp in labels) | ||
| { | ||
| labelEntries[index] = SampleLabelEntry.CreateSampleLabelEntry( | ||
| builder, | ||
| builder.CreateString(kvp.Key), | ||
| builder.CreateString(kvp.Value) | ||
| ); | ||
| index++; | ||
| } | ||
|
|
There was a problem hiding this comment.
CreateSampleLabelsVector builds the [SampleLabelEntry] vector by calling IncCounter.CreateLabelsVector(...), which unnecessarily couples label serialization to the presence/name of the IncCounter table. Consider building the vector directly with FlatBufferBuilder.StartVector/AddOffset (or introducing a dedicated helper) so label encoding remains independent of any specific message type.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| index++; | ||
| } | ||
|
|
||
| return IncCounter.CreateLabelsVector(builder, labelEntries); |
There was a problem hiding this comment.
The method uses IncCounter.CreateLabelsVector to create label vectors for all message types (IncCounter, SetGauge, ObserveHistogram). While this works because all generated FlatBuffer types have identical CreateLabelsVector methods, it's semantically confusing and reduces code clarity. Consider using a more neutral type like SetGauge.CreateLabelsVector or documenting why IncCounter is used here.
| return IncCounter.CreateLabelsVector(builder, labelEntries); | |
| return SetGauge.CreateLabelsVector(builder, labelEntries); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Implements counter, gauge and histogram types in the FFI layers