[Heartbeat] Custom policy reload#49326
Conversation
emilioalvap
added
enhancement
Team:obs-ds-hosted-services
botelastic
Bot
added
needs_team
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
TL;DR
Remediation
Investigation detailsRoot CauseThe run failed in step Evidence
Validation
Follow-upAfter applying fixes, push and re-run the same workflow to confirm all three What is this? | From workflow: PR Actions Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reloadable HBRunnerList with NewHBRunnerList and reload/stop/hash APIs; RunCentralMgmtMonitors now constructs an HB-specific runner list. Introduces HBRunnerFactory with GetHashFunc and makes runner updates possible via UpdatableRunner. Monitor now holds plugin.Plugin, uses plugin.Endpoints, and exposes Update(*conf.C) error. Plugin system gains HashConfigFunc, RegisterWithHashFunc, and Plugin.Update/DoUpdate hooks. Browser monitor registration now uses RegisterWithHashFunc with a hashConfig that ignores params. SourceJob adds mutex-protected Params and Update; synthexec APIs switch params to func() map[string]interface{}. Root config unnesting applied and revision removed. Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@heartbeat/reload/list.go`:
- Around line 124-140: The loop handling updateList currently skips
non-updatable runners and leaves changed same-hash configs ignored; modify the
non-updatable branch in the for hash,cfg := range updateList loop (the code that
checks if runner.(UpdatableRunner)) to behave like the Update error path: when
the existing runner in r.runners[hash] is not an UpdatableRunner, add the runner
to stopList[hash] and add the new cfg to startList[hash] (and keep a log via
r.logger.Errorf/Debugf), so changed configs for same-hash non-updatable runners
fall back to the stop/start cycle.
In `@x-pack/heartbeat/monitors/browser/sourcejob.go`:
- Around line 100-111: The Update method directly writes sj.browserCfg.Params
which is later read by the long‑lived synthexec job (method value captured
elsewhere), causing a race; fix by making Params access thread‑safe: add a
synchronization mechanism (either a sync.Mutex protecting browserCfg.Params or
use an atomic.Value field on SourceJob to hold a snapshot of Params), change
SourceJob.Update to store a deep copy/snapshot into that protected field (e.g.,
set atomic.Value.Store(copyOf(cfg.Params)) or lock mutex, replace params,
unlock), and update the synthexec job reader to load the params from the same
protected accessor (atomic.Value.Load() or mutex-protected getter) so readers
never observe partially updated state.
In `@x-pack/heartbeat/monitors/browser/synthexec/synthexec.go`:
- Around line 155-158: The code calls params() twice which can yield different
results; capture its result once into a local variable (e.g., p := params()) and
use that for the length check and the json.Marshal call, then append stringified
params to cmd.Args (using json.Marshal(p) and handling the error instead of
discarding it) so both checks operate on the same data and errors aren’t
ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d027687-51da-40cd-a771-a2dd7c854ae7
📒 Files selected for processing (13)
heartbeat/beater/heartbeat.goheartbeat/monitors/factory.goheartbeat/monitors/monitor.goheartbeat/monitors/plugin/plugin.goheartbeat/reload/list.goheartbeat/reload/list_test.gox-pack/heartbeat/cmd/root.gox-pack/heartbeat/monitors/browser/browser.gox-pack/heartbeat/monitors/browser/config.gox-pack/heartbeat/monitors/browser/config_test.gox-pack/heartbeat/monitors/browser/sourcejob.gox-pack/heartbeat/monitors/browser/sourcejob_test.gox-pack/heartbeat/monitors/browser/synthexec/synthexec.go
emilioalvap
added
backport-skip
|
@Mergifyio backport 9.2 9.3 |
emilioalvap
added
backport-skip
✅ Backports have been createdDetails
|
* Add heartbea reload and config hash logic Introducing: A config hashing mechanism to be optionally implemented by each of heartbeat plugin's (monitor types) to selective exclude fields from the hash generation. An alternative Update() route, to be implemented optionally by heartbeat plugins to selective update the configuration of a running monitor. Both of these mechanisms are required to dynamically update running monitors fields, browser params in particular, without triggering a stop/start cycle. (cherry picked from commit 9469ea9)
* Add heartbea reload and config hash logic Introducing: A config hashing mechanism to be optionally implemented by each of heartbeat plugin's (monitor types) to selective exclude fields from the hash generation. An alternative Update() route, to be implemented optionally by heartbeat plugins to selective update the configuration of a running monitor. Both of these mechanisms are required to dynamically update running monitors fields, browser params in particular, without triggering a stop/start cycle. (cherry picked from commit 9469ea9)
3 participants
Proposed commit message
Introducing:
Both of these mechanisms are required to dynamically update running monitors fields, browser
paramsin particular, without triggering a stop/start cycle.Closes #47511.
Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.How to test this PR locally
Related issues