remove global paths from metricbeat#51591
Conversation
🤖 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
|
3d91b63 to
32663e6
Compare
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
|
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:
📝 WalkthroughWalkthroughThis PR adds a changelog fragment and threads per-instance paths through Metricbeat construction, receiver startup, and SQL cursor registry code. Suggested labels: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
orestisfl
left a comment
There was a problem hiding this comment.
LGTM other than the changelog fragment
|
@elastic/obs-infraobs-integrations any chance you can review? |
This comment has been minimized.
This comment has been minimized.
|
@leehinman While we are at it, do you think we can migrate off global paths in the tests as well? |
Originally I was trying to "fix" quickly since this is a bug affecting users, but since we haven't gotten the external reviews yet and you have such a nice patch, I'll add it. Added in 48fc971 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x-pack/metricbeat/module/sql/query/cursor_integration_test.go`:
- Line 10: The test file has an unused context import that causes the build to
fail; remove the import from the import block in cursor_integration_test.go, or
if it is meant to be used, add it to the relevant test setup or helper functions
so the compiler sees an actual reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: daf634f9-9bc9-4b66-b0dd-96fc624c61e4
📒 Files selected for processing (1)
x-pack/metricbeat/module/sql/query/cursor_integration_test.go
c72ff75 to
d7c6ff1
Compare
|
This pull request is now in conflicts. Could you fix it? 🙏 |
d7c6ff1 to
f12b77f
Compare
TL;DR
Remediation
Investigation detailsRoot CauseThis is a code bug: one test helper was migrated from func NewModule(config *conf.C, r *Register, info beat.Info) (Module, []MetricSet, error)However, _, metricsets, err := mb.NewModule(c, mb.Registry, beatPaths, logptest.NewTestingLogger(t, ""))The same file already imports Evidence
Verification
What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
Tick the box to add this pull request to the merge queue (same as
|
|
@Mergifyio backport 9.5 9.4 9.3 8.19 |
❌ No backport have been createdDetails
GitHub error:
Cherry-pick of 37f3d26 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of 37f3d26 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of 37f3d26 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
* remove global paths from metricbeat (#51591) * remove global paths from metricbeat * x-pack/metricbeat/sql: migrate cursor tests off the global paths singleton --------- Co-authored-by: Orestis Floros <orestis.floros@elastic.co> (cherry picked from commit 37f3d26) # Conflicts: # metricbeat/mb/builders.go # x-pack/libbeat/cmd/instance/beat.go # x-pack/metricbeat/module/sql/query/query_test.go * fix merge conflicts --------- Co-authored-by: Lee E Hinman <57081003+leehinman@users.noreply.github.com> Co-authored-by: Lee E. Hinman <lee.e.hinman@elastic.co>
* remove global paths from metricbeat (#51591) * remove global paths from metricbeat * x-pack/metricbeat/sql: migrate cursor tests off the global paths singleton --------- Co-authored-by: Orestis Floros <orestis.floros@elastic.co> (cherry picked from commit 37f3d26) # Conflicts: # metricbeat/mb/builders.go # x-pack/libbeat/cmd/instance/beat.go # x-pack/metricbeat/module/sql/module.go # x-pack/metricbeat/module/sql/module_test.go # x-pack/metricbeat/module/sql/query/cursor/store_test.go # x-pack/metricbeat/module/sql/query/cursor_integration_test.go # x-pack/metricbeat/module/sql/query/query_integration_test.go # x-pack/metricbeat/module/sql/query/query_test.go * fix merge conflicts * remove files that shouldn't have been backported * remove files that shouldn't have been backported --------- Co-authored-by: Lee E Hinman <57081003+leehinman@users.noreply.github.com> Co-authored-by: Lee E. Hinman <lee.e.hinman@elastic.co>
* remove global paths from metricbeat (#51591) * remove global paths from metricbeat Co-authored-by: Lee E Hinman <57081003+leehinman@users.noreply.github.com> Co-authored-by: Lee E. Hinman <lee.e.hinman@elastic.co>
Proposed commit message
This change removes the use of global/singleton path state from Metricbeat and replaces it with per-instance paths, fixing a bug where multiple Metricbeat receivers could share the same data store.
Problem
Metricbeat was using global paths.Paths (a package-level singleton) when resolving file paths like the SQL cursor data store. In receiver mode, multiple Metricbeat instances share the same process, so they would all resolve to the same data path, causing them to share state incorrectly.
Changes
Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Disruptive User Impact
None. Stand alone Metricbeat will function as before, only metricbeat receiver will have multiple beat instances in the same process. This change will allow the sql module to have unique data path per beat instance.
How to test this PR locally
Related issues
Use cases
Screenshots
Logs