Skip to content

feat(graph_provider): rework x-medkit-graph health model (#545)#546

Merged
bburda merged 21 commits into
mainfrom
fix/545-graph-provider-health
Jul 23, 2026
Merged

feat(graph_provider): rework x-medkit-graph health model (#545)#546
bburda merged 21 commits into
mainfrom
fix/545-graph-provider-health

Conversation

@bburda

@bburda bburda commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

The x-medkit-graph resource reported wrong pipeline health. It could show a
healthy system as broken, and a broken one as healthy. The feature had
never produced a real graph edge in tests, because no demo publisher and
subscriber shared a topic, so many defects were never caught.

This PR replaces the status model with one based on data freshness, and fixes
the defects found once a real edge existed.

Key changes:

  • Status model is now pending / active / error (metrics_stale), driven
    by whether metrics are present and fresh. The old global "diagnostics seen"
    flag that turned any unmatched edge into broken is gone.
  • Freshness age uses a monotonic clock, so a backward clock step cannot mask a
    dead topic as fresh. A configurable stale_grace_sec debounces a single
    late /diagnostics sample so the pipeline does not flap.
  • frame_rate_msg from greenwave_monitor is the arrival rate summed across
    all publishers on a topic. A duplicate or leftover publisher inflates it and
    could hide a slow producer. Each edge now reports publisher_count and
    rate_ambiguous, and a new config multi_publisher_rate (annotate /
    suppress) controls how the plugin reacts.
  • metrics.source is the resolved publisher node name; it is cleared when the
    latest sample cannot be attributed, instead of showing a stale or hardcoded
    value.
  • Per-function config overrides now apply (they were silently dropped), config
    values are validated, the topic filter matches path segments not substrings,
    last_seen_by_app is bounded, and a dead stale-topic code path plus an
    unused cache are removed.
  • A cyclic-subscription sampler could be called after its plugin was torn
    down. The sampler registry now removes a plugin's samplers on teardown and
    drains any in-flight call.

schema_version is 2.0.0.

Issue

Type

  • Bug fix
  • New feature or tests
  • Breaking change
  • Documentation only

Breaking: error_reason values changed (only metrics_stale remains reachable
on an edge), metrics.source can now be omitted, new optional fields were
added, and pipeline_status reporting changed. Deployments that previously saw
broken from the old bug will now report healthy or degraded.

Testing

  • Unit tests for the freshness math, config validation, the topic filter, GID
    based source resolution, and the rate policies.
  • A new integration suite drives a real greenwave_monitor node feeding real
    /diagnostics through the gateway HTTP endpoint: metrics path, staleness,
    cyclic subscriptions over SSE, and function scoping.
  • Reviewers can run the metrics suite with the graph provider plugin loaded and
    a greenwave_monitor node, then GET
    /api/v1/functions/{id}/x-medkit-graph and check pipeline_status follows
    the producer state.
  • Docs build clean under -W.

Checklist

  • Breaking changes are clearly described (and announced in docs / changelog if needed)
  • Tests were added or updated if needed
  • Docs were updated if behavior or public API changed

Known limitations / follow-ups

  • Two separate /diagnostics producers reporting the same topic can mix field
    attribution on one edge. Uncommon setup; documented.

  • Per-function overrides are set via a params file (config_file), not a bare
    -p CLI argument, because the ROS parameter CLI cannot express a nested key
    with a hyphen in the function id. Documented in the config reference.

  • A standard diagnostic_updater producer uses different diagnostic keys and
    names the status after the task, not the topic, so it will not match. The
    producer contract (greenwave-style keys, DiagnosticStatus.name = the
    fully-qualified topic name) is documented.

  • A separate follow-up is needed for the client tool that consumes this
    endpoint; it currently parses a document shape this plugin does not emit.

  • A pre-existing TSan data race in the trigger subscription teardown path
    (unrelated to this change) surfaces on the sanitizer job. It is fixed in
    [BUG] TSan data race: trigger topic subscription callback races shutdown teardown #548, which lands first; this PR then rebases onto that fix.

@bburda
bburda force-pushed the fix/545-graph-provider-health branch from 2adb294 to f58ca4c Compare July 21, 2026 16:24
@bburda bburda self-assigned this Jul 21, 2026
@bburda bburda changed the title fix(graph_provider): rework x-medkit-graph health model (#545) feat(graph_provider): rework x-medkit-graph health model (#545) Jul 21, 2026
@bburda

bburda commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

@eclipse0922 you wrote the original x-medkit-graph provider, so your review here would be valuable, especially on the greenwave_monitor interop.

Short summary of what this adds to the plugin:

  • Pipeline status is now based on data freshness. Before, any /diagnostics traffic could turn an edge without matching metrics into an error and make the whole function read as broken. Now an edge is broken only when its own metrics actually went stale.
  • greenwave_monitor stays the reference producer. The plugin now handles a header-less topic (it falls back to frame_rate_node) and resolves metrics.source to the real publisher node name instead of a fixed string.
  • New operator controls: stale_grace_sec debounces a single late sample so the pipeline does not flap, and multi_publisher_rate (annotate / suppress) handles the case where more than one publisher shares a topic. frame_rate_msg is a topic-level rate summed over all publishers, so a duplicate or leftover publisher inflates it.
  • New per-edge fields publisher_count and rate_ambiguous so the operator can see when the rate may be inflated.
  • An integration suite now drives a real greenwave_monitor node through the gateway.

The producer contract (greenwave-style keys, DiagnosticStatus.name set to the fully-qualified topic name) is documented in the tutorial. If anything about the greenwave side looks wrong to you, please flag it.

@bburda
bburda marked this pull request as ready for review July 21, 2026 16:39
Copilot AI review requested due to automatic review settings July 21, 2026 16:39
Copilot AI reviewed Jul 21, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reworks the x-medkit-graph pipeline health model in ros2_medkit_graph_provider to be driven by metrics freshness (monotonic clock + debounce), fixes several latent defects that were previously untested due to missing real publisher/subscriber edges, and hardens cyclic-subscription sampler teardown to avoid use-after-free when plugins unload.

Changes:

  • Replace edge status semantics with freshness-based pending / active / error(metrics_stale) using a monotonic clock and configurable stale_grace_sec.
  • Add multi-publisher ambiguity reporting (publisher_count, rate_ambiguous) and a multi_publisher_rate policy to suppress/annotate ambiguous rates.
  • Add substantial new integration coverage (real greenwave_monitor, SSE sampler, scoping/filtering) plus corresponding docs updates.

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/ros2_medkit_plugins/ros2_medkit_graph_provider/src/graph_provider_plugin.cpp Implements freshness-based health model, monotonic timing, source resolution via GID, multi-publisher policy, config flattening/validation, and removes dead cache/stale-topic code.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/include/ros2_medkit_graph_provider/graph_provider_plugin.hpp Updates public plugin structures/config schema version, adds new config knobs and fields (source, timestamps, publisher counts).
src/ros2_medkit_plugins/ros2_medkit_graph_provider/README.md Updates plugin description, config keys/path, and documentation links to match new model and schema.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/design/index.rst Updates design doc to reflect new schema/version, freshness model, source attribution, FIFO cache behavior, and removed fault-manager integration.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/CHANGELOG.rst Documents breaking schema/semantics changes and new operator controls.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt Removes unused ros2_medkit_msgs dependency from plugin and tests.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/package.xml Removes unused ros2_medkit_msgs dependency.
src/ros2_medkit_integration_tests/test/features/test_graph_provider_plugin.test.py Updates schema_version assertion to 2.0.0.
src/ros2_medkit_integration_tests/test/features/test_graph_provider_greenwave.test.py New end-to-end suite exercising real /diagnostics metrics via greenwave_monitor, including pending vs active and degraded behavior.
src/ros2_medkit_integration_tests/test/features/test_graph_provider_stale.test.py New end-to-end stale/recovery coverage for metrics_stale and pipeline broken transitions.
src/ros2_medkit_integration_tests/test/features/test_graph_provider_sse.test.py New integration coverage for cyclic-subscription SSE sampler lifecycle and live pending→active transition.
src/ros2_medkit_integration_tests/test/features/test_graph_provider_scoping.test.py New integration coverage for per-function scoping and the segment-anchored NITROS topic filter behavior.
src/ros2_medkit_integration_tests/ros2_medkit_test_utils/launch_helpers.py Adds temp_monitor demo node registry entry and introduces create_greenwave_node() factory with launch-ordering guidance.
src/ros2_medkit_integration_tests/demo_nodes/engine_temp_monitor.cpp Adds a demo subscriber node so tests (and demos) can produce real pub/sub edges.
src/ros2_medkit_integration_tests/CMakeLists.txt Builds/installs the new demo node and increases timeouts for greenwave-driven integration tests.
src/ros2_medkit_integration_tests/package.xml Adds greenwave_monitor as a test dependency.
src/ros2_medkit_gateway/src/core/resource_sampler.cpp Adds safe sampler removal with draining of in-flight calls and liveness gating for already-copied callables.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/resource_sampler.hpp Documents and exposes remove_sampler()/collection_names() and the control-block model for safe teardown.
src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp Tracks samplers registered during set_context() and removes them on plugin disable/shutdown.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/plugins/plugin_manager.hpp Adds per-plugin sampler collection tracking and teardown helpers; removes unused register_resource_sampler API.
src/ros2_medkit_gateway/test/test_resource_sampler_registry.cpp Adds focused concurrency/lifecycle tests for sampler removal, copies, and in-flight drain behavior.
src/ros2_medkit_gateway/test/test_plugin_manager.cpp Adds tests ensuring plugin disable/shutdown removes plugin-registered samplers without affecting built-ins.
src/ros2_medkit_gateway/config/examples/demo_nodes_manifest.yaml Adds the new temp_monitor app and hosts it in the engine-monitoring function so demo graphs can form edges.
docs/tutorials/index.rst Adds the new graph-provider tutorial to the tutorials index.
docs/tutorials/graph-provider.rst Adds a new tutorial covering producer contract, activation, worked example, and SSE usage for x-medkit-graph.
docs/tutorials/plugin-system.rst Extends plugin-system tutorial with notes pointing to the graph provider config/tutorial and new freshness controls.
docs/config/index.rst Adds graph-provider configuration reference page to the config index.
docs/config/graph-provider.rst Adds a full configuration reference for freshness/debounce/multi-publisher policy plus overrides.
docs/api/rest.rst Updates x-medkit-graph schema_version/example/field notes to reflect new status model and fields.
docs/troubleshooting.rst Adds troubleshooting guidance for the common “all edges pending” case tied to /diagnostics producer contract.

@eclipse0922

eclipse0922 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Thanks for catching the major blind spot in my original implementation, @bburda! Missing real graph edges in the initial test suite was definitely a miss on my end, and refactoring the status model around per-edge data freshness is a much cleaner and more robust approach.

A few quick notes on the review:

Freshness & Debouncing: Moving away from the global diagnostic flag to per-edge freshness with stale_grace_sec makes total sense. Debouncing late samples will stop a lot of status flapping in real deployments.

Publisher Resolution: Handling header-less topics with the frame_rate_node fallback and dynamically resolving metrics.source nicely fixes the stale attribution issue.

Multi-publisher Handling: Exposing publisher_count and rate_ambiguous alongside multi_publisher_rate controls provides great operational visibility for aggregated rates.

Producer Contract: The contract requirements (greenwave-style keys, FQTN in DiagnosticStatus.name) line up well with what greenwave producers expect.

Overall, this redesign fixes the core architectural flaws while making the interop much more reliable. Looks great!

@bburda
bburda force-pushed the fix/545-graph-provider-health branch from b708679 to 7e462c9 Compare July 22, 2026 08:37
@bburda
bburda requested a review from mfaferek93 July 22, 2026 10:32
@bburda
bburda force-pushed the fix/545-graph-provider-health branch from 7e462c9 to e921cf5 Compare July 23, 2026 08:53
mfaferek93
mfaferek93 reviewed Jul 23, 2026
View reviewed changes
Comment thread src/ros2_medkit_plugins/ros2_medkit_graph_provider/src/graph_provider_plugin.cpp
Comment thread src/ros2_medkit_plugins/ros2_medkit_graph_provider/src/graph_provider_plugin.cpp Outdated
bburda added 17 commits July 23, 2026 16:46
@bburda
test(integration_tests): add engine temperature monitor demo node
6ad0518 
Gives the demo stack a real publisher/subscriber pair on one topic inside one
Function, so dataflow-graph resources actually produce an edge.
@bburda
fix(gateway): remove plugin samplers on teardown and drain in-flight …
38fdcd1 
…calls

A cyclic-subscription sampler captures its plugin instance. The sampler
registry now drops a plugin's samplers when the plugin is torn down and drains
any call already running, so a tick cannot run against a freed plugin.
@bburda
fix(graph_provider): replace the status model with data freshness
2d396d4 
Removes the global diagnostics-seen flag that reported healthy systems as
broken. metrics_status is now pending / active / error(metrics_stale), and an
edge is broken only when its metrics went stale.
@bburda
fix(graph_provider): apply per-function config overrides and bound state
d43ff9f 
Per-function overrides were delivered nested and silently dropped; they now
apply. Config values are validated. last_seen_by_app is bounded and the
write-only graph cache is removed.
@bburda
refactor(graph_provider): remove the never-executed stale-topic path
78946ea 
Both callers disabled it, so the fault-manager correlation never ran.
Removing it drops ros2_medkit_msgs as a dependency of this package.
@bburda
fix(graph_provider): anchor the negotiation topic filter to path segm…
7d1feb2 
…ents

Substring matching wrongly dropped real topics like /nitros_bridge/data. The
filter now matches nitros as a full path segment and _supported_types as a
trailing segment.
@bburda
feat(graph_provider): set x-medkit-graph schema_version to 2.0.0
cf3548e 
The wire contract changed (error_reason values, metrics_status semantics).
The version literal becomes a named constant.
@bburda
fix(graph_provider): resolve real metrics.source from the publisher
d824e8a 
Resolves the /diagnostics publisher node name by GID match instead of a
hardcoded literal, and omits the field when the sample cannot be attributed.
@bburda
test(integration_tests): drive real greenwave_monitor through the gat…
9130526 
…eway

Adds suites that feed real /diagnostics metrics through the gateway HTTP
endpoint: metrics path, staleness, cyclic subscriptions over SSE, and function
scoping.
@bburda
docs: document the graph provider plugin
f473b90 
Adds a tutorial, config reference, and troubleshooting entry, and corrects the
API reference and design doc to match the reworked behaviour.
@bburda
fix(graph_provider): monotonic freshness clock and stateless stale-grace
2ad55e7 
Freshness age uses a monotonic clock so a backward clock step cannot mask a
dead topic. A configurable stale_grace_sec debounces a single late sample; the
verdict is a stateless function of age, window, and grace.
@bburda
fix(graph_provider): defend against multi-publisher rate inflation
64b9385 
frame_rate_msg is a topic-level rate summed over all publishers, so a duplicate
publisher inflates it. Each edge reports publisher_count and rate_ambiguous;
multi_publisher_rate (annotate/suppress) controls the response.
@bburda
fix(graph_provider): warn on invalid config overrides and guard shutdown
779756c 
An override with an unknown field name or a wrong value type now warns instead
of being dropped silently. introspect() gets a shutdown guard.
@bburda
docs: document robustness fields and update stale docs
b476f49 
Documents publisher_count, rate_ambiguous, multi_publisher_rate, and
stale_grace_sec, and fixes remaining stale docs (drop-rate caveat, timestamp
precision, the worked example, producer contract, last_seen).
@bburda
fix(graph_provider): sharpen config validation warnings
1f4f1f2 
Reorder the per-function override loop in load_parameters so an
unrecognized field name is always reported as unrecognized, regardless
of its value's JSON type. Previously a field that was both unknown and
given a non-numeric value hit the numeric type check first and warned
"expected a number", masking the more useful diagnosis that the field
name itself was never going to apply. The field-name recognition gate
now runs before any value-type check; a recognized field still reaches
its own type-specific validation exactly as before.

Also warn when a GLOBAL (non-override) plugin config value is present
with the wrong JSON type instead of silently falling back to the
default with no signal, matching the per-function override behavior.
An absent key remains silent, since absence is normal.
@bburda
docs(gateway): document remove_sampler caller-blocking contract
eaf3339 
Document that ResourceSamplerRegistry::remove_sampler() blocks until
any in-flight invocation of that sampler callable returns. Callers
(e.g. PluginManager::disable_plugin, which holds plugins_mutex_ for
the duration of the call) must not hold a lock that the sampler
callable could itself need to acquire, or the two deadlock. The only
in-tree sampler callable today (graph_provider's x-medkit-graph
sampler) takes no such lock, so this is currently safe; the comment
protects a future plugin author who adds one that does.
@bburda
test(graph_provider): wait for DDS discovery in source-resolution tests
9003927 
metrics.source resolves via a fresh get_publishers_info_by_topic query
against the plugin node's graph cache, matched by publisher GID. Both the
unit tests and the greenwave acceptance test published a message and
asserted the resolved source right after a fixed spin+sleep, racing DDS
discovery: under CI latency the message can be processed before its
publisher's endpoint is visible in the graph cache, so source is (correctly)
omitted and the assertion flakes.

Add a bounded wait that spins and re-checks get_publishers_info_by_topic
until the expected publisher(s) are actually discovered, with a loud timeout
failure instead of a longer fixed sleep, and apply it before publishing the
message each test asserts on. The unresolved half of the clear-on-unresolved
test needs no wait, since a bogus GID never matches by construction. Extend
the acceptance test's poll to wait for metrics.source specifically, since it
can lag the edge's first active sample by a discovery cycle.

Each test still proves exactly what it did before: per-message GID
resolution across two simultaneous publishers, clearing on an unresolved
later sample while freshness still advances, and resolution to the
publisher's real fully-qualified name.
bburda added 4 commits July 23, 2026 16:46
@bburda
test(integration_tests): drop rclpy.parameter_client for Humble compa…
e829372 
…tibility

test_graph_provider_greenwave and test_graph_provider_stale imported
rclpy.parameter_client.AsyncParameterClient to live-tune a demo node's
publish_rate mid-suite. That module only exists on Jazzy+, so both test
files fail to even load on Humble ("No module named
'rclpy.parameter_client'"), failing the whole suite before a single test
runs.

Replace it with a direct rcl_interfaces/srv/SetParameters service client
against the target node's own set_parameters service - the same mechanism
AsyncParameterClient wraps internally, using only core rclpy/rcl_interfaces
APIs available on every supported distro. Behavior is unchanged: same
service name, same request/response shape, same synchronous wait-then-spin
pattern.
@bburda
fix(graph_provider): resolve metrics.source across RMWs via single-pu…
0ab9610 
…blisher fallback

metrics.source was resolved only by matching the sample's publisher GID
against the /diagnostics endpoint GIDs from the ROS graph. That match is
RMW dependent: on rmw_fastrtps_cpp both are the DDS GUID and match, but on
rmw_cyclonedds_cpp the message publisher_gid is a DDS instance handle, not
the endpoint GUID, so it never matches and source was always omitted there.

Add an RMW-agnostic fallback: when exactly one publisher exists on
/diagnostics, any sample on it must have come from that publisher, so
attribute it without relying on GID comparison. This resolves the common
case (one producer, e.g. greenwave_monitor) on every RMW. Per-sample
attribution across several simultaneous /diagnostics publishers still needs
an RMW whose message GID equals the endpoint GID; on an RMW without that,
a sample from one of several publishers is left unattributed (omitted)
rather than guessed. No RMW is special-cased - the code path is identical
everywhere and resolves more or less depending on what the RMW exposes.

The source-resolution unit tests now assert the RMW-agnostic invariant
(source is never the wrong publisher; single-publisher cases resolve on
both RMWs; the clear-on-unresolved case uses two publishers to make the
unattributable sample deterministic). Verified green under both
rmw_fastrtps_cpp and rmw_cyclonedds_cpp, unit and greenwave integration.
@bburda
test(integration): expect engine-temp-monitor app in hybrid suppressi…
3c07302 
…on test

The demo manifest defines the engine-temp-monitor App (the graph-provider
consumer that subscribes to /powertrain/engine/temperature, giving the
demo a real publisher/subscriber edge). The hybrid suppression test asserts
exact entity counts against a hardcoded expected set that did not include
it, so it failed with an extra app.

Add engine-temp-monitor to the expected app set and launch its bound node
(temp_monitor) alongside ALL_DEMO_NODES so the manifest app links to a live
runtime node and reaches online state.
@bburda
fix(graph_provider): fold node reachability into pipeline_status
688b225 
pipeline_status was rolled up from edges only, so a Function with a dead
scoped node read as "healthy": an offline app carries no topics, produces no
edge, and the surviving edges stay active. Fold node reachability in - any
unreachable scoped node now makes pipeline_status at least "degraded" (a
stale edge still outranks it as "broken"), and node_status names which node.

Also correct the multi_publisher kSuppress comment and docs. Suppressing an
ambiguous rate removes the misleading number, not the health verdict, so it
does not by itself stop a broken pipeline from reading healthy - rate_ambiguous
already flags the ambiguity.

Add unit tests (build and HTTP-handler paths) and an end-to-end launch test
that kills a manifest-scoped node and asserts the served graph goes from
healthy to degraded with that node unreachable.
@bburda
bburda force-pushed the fix/545-graph-provider-health branch from a58b7bc to 688b225 Compare July 23, 2026 14:48
@bburda
bburda merged commit d5a7311 into main Jul 23, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@mfaferek93 mfaferek93 mfaferek93 approved these changes

Assignees

@bburda bburda

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Robustness and operator controls for x-medkit-graph health [BUG] x-medkit-graph reports wrong pipeline health

4 participants

@bburda @eclipse0922 @mfaferek93