feat: Hide telemetry and discovery for nodes present in HIDDEN_CHANNELS and outside ALLOWED_CHANNELS - #781
feat: Hide telemetry and discovery for nodes present in HIDDEN_CHANNELS and outside ALLOWED_CHANNELS#781CypressXt wants to merge 3 commits into
HIDDEN_CHANNELS and outside ALLOWED_CHANNELS#781Conversation
CypressXt
changed the title
HIDDEN_CHANNELS and outside ALLOWED_CHANNELSHIDDEN_CHANNELS and outside ALLOWED_CHANNELS
There was a problem hiding this comment.
Hello, thanks for your contribution!
The combined check not is_allowed && hidden is fine, but there is a subtle case worth a comment: when ALLOWED_CHANNELS is non-empty but HIDDEN_CHANNELS is empty and the channel is unknown (channel_name() → None), is_allowed_channel(None) returns False, so the node/telemetry is silently dropped. That's probably intentional ("if you opted into an allow-list, be strict") but it should be documented in README/PROMETHEUS.md alongside the env var description, and explicitly tested.
Items worth addressing: (1) flip the missing-channel default so unknown-channel nodes are not silently dropped, (2) extract the filter into channels.is_filtered().
Having some tests would be great to confirm the logic but that's no blocker.
| ) | ||
| return | ||
| else: | ||
| return |
There was a problem hiding this comment.
payload[node_id] is the result of _node_to_dict(node) (serialization.py:281). Meshtastic node objects from the snapshot path (daemon._try_send_snapshot, daemon.py:412) and from MeshCore (protocols/meshcore/handlers.py:74,114,143) frequently do not contain a "channel" key. The new else: return then drops every such node whenever either filter list is configured. Net effect: enabling ALLOWED_CHANNELS or HIDDEN_CHANNELS likely causes the node table to stop growing entirely.
There was a problem hiding this comment.
The fix should be the opposite default: when the channel is unknown, fall through to the existing _queue_post_json (consistent with how store_telemetry_packet keeps channel = 0 when missing). At minimum, extract the channel index before applying the filter, and only suppress when a channel is known and excluded.
There was a problem hiding this comment.
Even the "channel is present" path is fragile: channel_name(idx) returns None for any index that hasn't been registered yet (race with capture_from_interface). With ALLOWED_CHANNELS set, is_allowed_channel(None) is False → all unmapped channels get dropped silently.
|
|
||
| if len(channels.allowed_channel_names()) > 0 or len(channels.hidden_channel_names()) > 0: | ||
| channel_name = channels.channel_name(channel) | ||
| if not channels.is_allowed_channel(channel_name) or channels.is_hidden_channel(channel_name): |
There was a problem hiding this comment.
Both handlers/generic.py and handlers/telemetry.py repeat the same len(...) > 0 or len(...) > 0 + not is_allowed_channel(...) or is_hidden_channel(...) pattern. Try to keep code as modular as possible to reduce duplication, this belongs in a single helper in channels.py, e.g. channels.is_filtered(channel_name) -> bool. That also makes the policy testable and consistent.
| ALLOWED_CHANNELS: ${ALLOWED_CHANNELS:-""} | ||
| HIDDEN_CHANNELS: ${HIDDEN_CHANNELS:-""} | ||
| ALLOWED_CHANNELS: ${ALLOWED_CHANNELS:-} | ||
| HIDDEN_CHANNELS: ${HIDDEN_CHANNELS:-} |
There was a problem hiding this comment.
That's unrelated but a good catch 👍
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Also, before you continue working on this, it is worth to discuss if we want to move the entire "HIDDEN_/ALLOWED_*" logic to the web server, see #784 |
|
Hey @l5yth, thanks a lot for the review ! |
2 participants
Hello there 👋,
Thanks a lot for the awesome project !
One thing that felt a bit confusing was the fact that you could set
ALLOWED_CHANNELSorHIDDEN_CHANNELSbut that excluded nodes and their telemetry would still appear in the node list.From my understanding the
ALLOWED_CHANNELSorHIDDEN_CHANNELSonly affect messages.Would it also make sense to "hide" telemetry and node discovery happening on those selected channels ?
Cheers 🖖