Send step and project context to monitor for metric tagging#3093
Conversation
The monitor receives flow-level tags at init time but has no visibility into per-step context (step_name, project_name, branch_name, is_production, is_user_branch). This sends those as a MUST_SEND context update at the start of run_step so that monitor implementations can include them as metric tags. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Greptile SummaryThis PR propagates step-level and project-level context ( Confidence Score: 5/5Safe to merge — the change is well-scoped, both previously flagged concerns are resolved, and no new issues are present. The boolean serialization bug raised in the prior review thread was fixed in the follow-up commit via explicit str() conversion. The is_active guard asymmetry is not a correctness bug (confirmed by reading Sidecar.send() which guards internally). The None-filtering, MUST_SEND choice, and post-current._set_env() placement are all correct. No remaining P0 or P1 findings. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "Fix boolean tag values being dropped by ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }.items() | ||
| if v is not None | ||
| } | ||
| self.monitor.send(Message(MessageTypes.MUST_SEND, monitor_tags)) |
There was a problem hiding this comment.
send() skips the is_active guard present in count() / measure()
NullMonitor.count() and NullMonitor.measure() wrap their payload construction in if self._sidecar.is_active:, avoiding the overhead of building and serialising a message when no sidecar is running. NullMonitor.send() delegates directly to Sidecar.send(), which does guard internally, so this is not a correctness bug — but the inconsistency means a no-op path still serialises monitor_tags into a Message object before the check fires.
If the pattern is intentional (because send() is caller-constructed), a short comment on NullMonitor.send() documenting that the is_active check is deferred to Sidecar.send() would prevent future confusion.
spectator's validate_tags only accepts string values — booleans from
current.get("is_production") and current.get("is_user_branch") were
silently stripped. Apply str(v) in the comprehension so booleans become
"True"/"False" strings while None values are still filtered out.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
2 participants
Summary
step_name,project_name,branch_name,is_production, andis_user_branchas aMUST_SENDcontext update to the monitor at the start ofrun_stepNonevalues to avoid invalid tagsMotivation
The monitor currently only receives flow-level context (flow_name, username, metaflow_version, etc.) at init time. Per-step context like
step_nameand project metadata are available inrun_stepbut never propagated to the monitor. This makes it impossible to slice metrics by step or project without custom workarounds.Test plan
NullMonitor.send()(base class) forwards the message to the sidecar without errorsend()receive the new tags🤖 Generated with Claude Code