fix: subjects #3078

Merged

mfreeman451 merged 1 commit from refs/pull/3078/head into staging

2026-03-25 23:22:29 +00:00

mfreeman451 commented

2026-03-25 23:21:03 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #3079
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/3079
Original created: 2026-03-25T23:21:03Z
Original updated: 2026-03-25T23:25:38Z
Original head: carverauto/serviceradar:3074-buglog-collector-failing-to-publish-traces
Original base: staging
Original merged: 2026-03-25T23:22:29Z by @mfreeman451

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

Imported from GitHub pull request. Original GitHub pull request: #3079 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/3079 Original created: 2026-03-25T23:21:03Z Original updated: 2026-03-25T23:25:38Z Original head: carverauto/serviceradar:3074-buglog-collector-failing-to-publish-traces Original base: staging Original merged: 2026-03-25T23:22:29Z by @mfreeman451 --- ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test?

Copilot commented

2026-03-25 23:25:37 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632942
Original created: 2026-03-25T23:25:37Z
Original path: rust/otel/src/nats_output.rs
Original line: 315

reconcile_subjects removes existing subjects when a required wildcard matches them (subject_matches(required, existing)). With the current subject_matches implementation, patterns like foo.> also match foo (because > returns true without requiring an additional token), which is not NATS wildcard semantics and can cause this reconciliation step to drop the exact subject (e.g., otel.metrics) and stop capturing messages published to that subject. Consider fixing subject_matches so > requires at least one remaining token for patterns like foo.>, and/or make reconciliation preserve exact subjects unless they are truly covered by the required wildcard per NATS rules.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632942 Original created: 2026-03-25T23:25:37Z Original path: rust/otel/src/nats_output.rs Original line: 315 --- `reconcile_subjects` removes existing subjects when a required wildcard matches them (`subject_matches(required, existing)`). With the current `subject_matches` implementation, patterns like `foo.>` also match `foo` (because `>` returns true without requiring an additional token), which is not NATS wildcard semantics and can cause this reconciliation step to drop the exact subject (e.g., `otel.metrics`) and stop capturing messages published to that subject. Consider fixing `subject_matches` so `>` requires at least one remaining token for patterns like `foo.>`, and/or make reconciliation preserve exact subjects unless they are truly covered by the required wildcard per NATS rules.

Copilot commented

2026-03-25 23:25:37 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632958
Original created: 2026-03-25T23:25:37Z
Original path: rust/otel/src/nats_output.rs
Original line: 320

The any(|existing| subject_matches(existing, required)) check is being applied even when required contains wildcards (e.g. otel.metrics.>). Since subject_matches treats wildcards in the subject argument as literals, a narrower existing wildcard like otel.metrics.* would incorrectly be considered to “cover” otel.metrics.> (because * matches the literal token >), which can prevent adding the required wildcard. Consider changing the coverage check so that wildcard required subjects are only treated as satisfied by an exact match, or implement a proper pattern-subsumption check for pattern-vs-pattern comparisons.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632958 Original created: 2026-03-25T23:25:37Z Original path: rust/otel/src/nats_output.rs Original line: 320 --- The `any(|existing| subject_matches(existing, required))` check is being applied even when `required` contains wildcards (e.g. `otel.metrics.>`). Since `subject_matches` treats wildcards in the `subject` argument as literals, a narrower existing wildcard like `otel.metrics.*` would incorrectly be considered to “cover” `otel.metrics.>` (because `*` matches the literal token `>`), which can prevent adding the required wildcard. Consider changing the coverage check so that wildcard `required` subjects are only treated as satisfied by an exact match, or implement a proper pattern-subsumption check for pattern-vs-pattern comparisons.

Copilot commented

2026-03-25 23:25:38 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632982
Original created: 2026-03-25T23:25:38Z
Original path: rust/otel/src/nats_output.rs
Original line: 387

The stream-subject reconciliation/update block is duplicated in both the get_or_create_stream success path and the fetch-and-update fallback. Consider extracting this into a small helper (e.g., returning (updated_subjects, removed_subjects) / needs_update) so future changes to reconciliation logic don’t accidentally diverge between the two code paths.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632982 Original created: 2026-03-25T23:25:38Z Original path: rust/otel/src/nats_output.rs Original line: 387 --- The stream-subject reconciliation/update block is duplicated in both the `get_or_create_stream` success path and the fetch-and-update fallback. Consider extracting this into a small helper (e.g., returning `(updated_subjects, removed_subjects)` / `needs_update`) so future changes to reconciliation logic don’t accidentally diverge between the two code paths.

Copilot commented

2026-03-25 23:25:38 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632987
Original created: 2026-03-25T23:25:38Z
Original path: rust/otel/src/nats_output.rs
Original line: 1266

This test expects otel.traces/otel.metrics to be removed when adding otel.traces.>/otel.metrics.>, which only holds if foo.> is treated as matching foo. If the intent is NATS wildcard semantics, foo.> should not match foo, so reconciliation should either retain the exact subjects or explicitly include them in the required list. Please align the test (and reconciliation logic) with the intended NATS/JetStream matching rules and add a regression assertion covering the foo.> vs foo case.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3079#discussion_r2991632987 Original created: 2026-03-25T23:25:38Z Original path: rust/otel/src/nats_output.rs Original line: 1266 --- This test expects `otel.traces`/`otel.metrics` to be removed when adding `otel.traces.>`/`otel.metrics.>`, which only holds if `foo.>` is treated as matching `foo`. If the intent is NATS wildcard semantics, `foo.>` should not match `foo`, so reconciliation should either retain the exact subjects or explicitly include them in the required list. Please align the test (and reconciliation logic) with the intended NATS/JetStream matching rules and add a regression assertion covering the `foo.>` vs `foo` case.