fix(appolly): preserve other selector criteria with pid matches#1911
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #1911 +/- ##
==========================================
- Coverage 78.23% 69.23% -9.00%
==========================================
Files 278 279 +1
Lines 34240 34283 +43
==========================================
- Hits 26786 23735 -3051
- Misses 6205 9279 +3074
- Partials 1249 1269 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adjusts AppO11y process discovery selector semantics so target_pids narrows matches rather than short-circuiting other selector criteria, aligning runtime behavior with documented configuration expectations.
Changes:
- Update
Matcher.matchProcessto treat PID lists as a required filter (fail fast on PID mismatch) while still evaluating other selector criteria. - Add a regression test ensuring PID matches do not bypass other configured criteria (e.g.,
exe_path).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/appolly/discover/matcher.go | Changes PID handling from early-return “match-by-PID only” to “PID must match, then evaluate remaining criteria”. |
| pkg/appolly/discover/matcher_test.go | Adds regression coverage verifying PID hits still require other selector properties (e.g., path) to match. |
|
sorry about the confusion, I looked back and we did this purposefully because PID seemed pretty explicit: #1321 (comment) but I agree with the revert to match the documentation and other behavior |
Summary
Why
Process matching returned early when a selector included
target_pids, which let a PID hit short-circuit the rest of the selector criteria. That made mixed selectors behave too broadly whenever a matching PID was present.This behavior also conflicted with the documented selector semantics. The generated OBI config reference describes
target_pidsas additive: the PID must match "in addition to any path/port criteria". The published OBI service discovery docs also state that when multiple selectors are configured in the sameinstrumententry, the process must match all selector properties.Impact
This keeps selector evaluation consistent with the documented configuration model: a configured PID list can narrow matching, but it no longer overrides other path, language, command-argument, container, or metadata criteria.
Validation
go test ./pkg/appolly/discover