feat: collect metrics + impact metrics flatbuffers + ygg ffi#81
feat: collect metrics + impact metrics flatbuffers + ygg ffi#81
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to collect both Unleash client metrics and impact metrics through a single FFI call. The implementation introduces a new flatbuffer message type CollectMetricsResponse and an FFI function flat_collect_metrics that gathers both types of metrics, serializes them to JSON, and returns them wrapped in a flatbuffer.
Changes:
- Added
CollectMetricsResponseflatbuffer schema definition for returning combined metrics data - Implemented
flat_collect_metricsFFI function that collects both impact metrics and toggle metrics - Created
MetricMeasurementandMetricsBucketstructs to represent the combined metrics data - Generated Rust and C# code from updated flatbuffer definitions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| flat-buffer-defs/enabled-message.fbs | Added CollectMetricsResponse table definition with response and error fields |
| yggdrasilffi/src/flat/enabled-message_generated.rs | Generated Rust code for CollectMetricsResponse flatbuffer structure |
| dotnet-engine/Yggdrasil.Engine/yggdrasil/messaging/CollectMetricsResponse.cs | Generated C# code for CollectMetricsResponse flatbuffer structure |
| yggdrasilffi/src/flat/serialisation.rs | Added MetricsBucket and MetricMeasurement structs, implemented FlatMessage trait for CollectMetricsResponse with JSON serialization |
| yggdrasilffi/src/flat/mod.rs | Implemented flat_collect_metrics FFI function that collects both metrics types and returns them as a flatbuffer |
Comments suppressed due to low confidence (1)
flat-buffer-defs/enabled-message.fbs:210
- CollectMetricsResponse is missing from the root_type declarations at the end of the schema file. This means the generated code will not include helper functions like root_as_collect_metrics_response, which are provided for other response types like VoidResponse, MetricsResponse, and TakeStateResponse. Since CollectMetricsResponse is being used as a root type (returned from flat_collect_metrics FFI function), it should be added to the root_type declarations. Add "root_type CollectMetricsResponse;" after line 210.
root_type Response;
root_type ContextMessage;
root_type MetricsResponse;
root_type TakeStateResponse;
root_type VoidResponse;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub unsafe extern "C" fn flat_collect_metrics(engine_ptr: *mut c_void) -> Buf { | ||
| let result = guard_result::<MetricMeasurement, _>(|| { | ||
| let guard = get_engine(engine_ptr)?; | ||
| let mut engine = recover_lock(&guard); | ||
| let impact_metrics = engine.collect_impact_metrics(); | ||
| let bucket = engine.get_metrics(Utc::now()); | ||
| match bucket { | ||
| Some(bucket) => Ok(Some(MetricMeasurement { | ||
| metrics: Some(bucket), | ||
| impact_metrics, | ||
| })), | ||
| None => Ok(Some(MetricMeasurement { | ||
| metrics: None, | ||
| impact_metrics, | ||
| })), | ||
| } | ||
| }); | ||
|
|
||
| CollectMetricsResponse::build_response(result) | ||
| } |
There was a problem hiding this comment.
The new flat_collect_metrics function lacks test coverage. While other flat functions like flat_take_state and flat_check_enabled have comprehensive tests (see lines 394-604 in this file), metrics-related functions appear to lack tests. Consider adding a test that verifies the function correctly collects both metrics and impact_metrics, handles the case where metrics is None, and properly serializes the response to JSON within the flatbuffer.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub unsafe extern "C" fn flat_collect_metrics(engine_ptr: *mut c_void) -> Buf { | ||
| let result = guard_result::<MetricMeasurement, _>(|| { | ||
| let guard = get_engine(engine_ptr)?; | ||
| let mut engine = recover_lock(&guard); | ||
| let impact_metrics = engine.collect_impact_metrics(); | ||
| let bucket = engine.get_metrics(Utc::now()); | ||
| match bucket { | ||
| Some(bucket) => Ok(Some(MetricMeasurement { | ||
| metrics: Some(bucket), | ||
| impact_metrics, | ||
| })), | ||
| None => Ok(Some(MetricMeasurement { | ||
| metrics: None, | ||
| impact_metrics, | ||
| })), | ||
| } | ||
| }); | ||
|
|
||
| CollectMetricsResponse::build_response(result) | ||
| } |
There was a problem hiding this comment.
A new FFI entrypoint (flat_collect_metrics) is introduced, but the existing test module in this file doesn’t cover it. Please add at least one unit/integration test that calls flat_collect_metrics, parses the returned FlatBuffer as CollectMetricsResponse, and asserts the error/response contract (including JSON shape).
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.
github-project-automation
bot
moved this from In Progress
to Approved PRs
in Issues and PRs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Debug, Clone, Deserialize, Serialize)] | ||
| pub struct MetricMeasurement { | ||
| pub metrics: Option<MetricBucket>, | ||
| pub impact_metrics: Vec<CollectedMetric>, | ||
| } |
There was a problem hiding this comment.
MetricMeasurement derives Deserialize but this module only serializes it to JSON. Keeping Deserialize adds an unnecessary trait requirement on MetricBucket/CollectedMetric and can break compilation if those upstream types don’t implement Deserialize. Consider dropping Deserialize (and the Deserialize import) unless you actually need to parse this struct from JSON somewhere.
| let metrics_str = serde_json::to_string(&measurement); | ||
| match metrics_str { | ||
| Err(error) => { | ||
| let error_offset = builder.create_string(&error.to_string()); | ||
| let mut response_builder = CollectMetricsResponseBuilder::new(builder); | ||
| response_builder.add_error(error_offset); | ||
| response_builder.finish() | ||
| } | ||
| Ok(m_str) => { | ||
| let collect_response = builder.create_string(&m_str); | ||
| let mut response_builder = CollectMetricsResponseBuilder::new(builder); | ||
| response_builder.add_response(collect_response); | ||
| response_builder.finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
This response embeds a JSON string (serde_json::to_string) inside a FlatBuffer. That’s inconsistent with the rest of the flat-FFI API (which returns typed FlatBuffer tables) and adds extra allocation/serialization overhead for potentially large payloads. Consider modeling the payload as FlatBuffer fields (e.g., reuse/extend MetricsResponse and the existing impact-metric tables) instead of JSON-in-a-string so consumers can parse it without a second deserialization pass.
| let metrics_str = serde_json::to_string(&measurement); | |
| match metrics_str { | |
| Err(error) => { | |
| let error_offset = builder.create_string(&error.to_string()); | |
| let mut response_builder = CollectMetricsResponseBuilder::new(builder); | |
| response_builder.add_error(error_offset); | |
| response_builder.finish() | |
| } | |
| Ok(m_str) => { | |
| let collect_response = builder.create_string(&m_str); | |
| let mut response_builder = CollectMetricsResponseBuilder::new(builder); | |
| response_builder.add_response(collect_response); | |
| response_builder.finish() | |
| } | |
| } | |
| // Build a typed MetricsResponse FlatBuffer table directly from the | |
| // MetricMeasurement instead of serializing it to a JSON string. | |
| let metrics_response = | |
| MetricsResponse::as_flat_buffer(builder, measurement); | |
| let mut response_builder = CollectMetricsResponseBuilder::new(builder); | |
| response_builder.add_response(metrics_response); | |
| response_builder.finish() |
| pub unsafe extern "C" fn flat_collect_metrics(engine_ptr: *mut c_void) -> Buf { | ||
| let result = guard_result::<MetricMeasurement, _>(|| { | ||
| let guard = get_engine(engine_ptr)?; | ||
| let mut engine = recover_lock(&guard); | ||
| let impact_metrics = engine.collect_impact_metrics(); | ||
| let bucket = engine.get_metrics(Utc::now()); | ||
| if bucket.is_none() && impact_metrics.is_empty() { | ||
| return Ok(None); | ||
| } | ||
| Ok(Some(MetricMeasurement { | ||
| metrics: bucket, | ||
| impact_metrics, | ||
| })) | ||
| }); | ||
|
|
||
| CollectMetricsResponse::build_response(result) | ||
| } |
There was a problem hiding this comment.
New FFI entrypoint flat_collect_metrics doesn’t have any test coverage, while this module already has a unit test suite for other flatbuffer endpoints. Add tests that (1) verify the returned buffer parses as CollectMetricsResponse, (2) validate the response JSON is present when metrics/impact metrics exist, and (3) validate the empty-table behavior when both are absent.
flatbuffers defs (+ rust & csharp generated) for CollectImpactMetricsResponse, ygg ffi method that collects impact metrics + unleash metrics, serializes to json then puts that in a flatbuffer object to return as Buf