Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "let the system/process metricset monitor a single PID (#39620)" #39714

Merged
merged 1 commit into from
May 24, 2024

Conversation

pchila
Copy link
Member

@pchila pchila commented May 24, 2024

Proposed commit message

This reverts commit 43d5dd4 introduced with PR #39620.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@pchila pchila requested review from a team as code owners May 24, 2024 09:26
@pchila pchila requested review from fearful-symmetry and faec May 24, 2024 09:26
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 24, 2024
@pchila pchila self-assigned this May 24, 2024
Copy link
Contributor

mergify bot commented May 24, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @pchila? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@pchila pchila added Team:Elastic-Agent Label for the Agent team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels May 24, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

1 similar comment
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

1 similar comment
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 24, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@rdner
Copy link
Member

rdner commented May 24, 2024

The failure seems to be caused by updating github.com/coreos/go-systemd/v22

https://github.com/coreos/go-systemd/blob/7d375ecc2b092916968b5601f74cca28a8de45dd/internal/dlopen/dlopen_example.go#L54

according to our error message:

dlopen_example.cgo2.c:(.text+0x36): multiple definition of `my_strlen'; /tmp/go-link-1331529901/000031.o:dlopen_example.cgo2.c:(.text+0x36): first defined here

@pchila
Copy link
Member Author

pchila commented May 24, 2024

The failure seems to be caused by updating github.com/coreos/go-systemd/v22

https://github.com/coreos/go-systemd/blob/7d375ecc2b092916968b5601f74cca28a8de45dd/internal/dlopen/dlopen_example.go#L24

according to our error message:

dlopen_example.cgo2.c:(.text+0x36): multiple definition of `my_strlen'; /tmp/go-link-1331529901/000031.o:dlopen_example.cgo2.c:(.text+0x36): first defined here

Interesting... in go.mod and go.sum of PR #39620 we can see that go-systemd has been updated
image

And in go.sum we see 2 versions of go-systemd but to have a duplicate definition we need to be linking the same symbol twice so:

  • either the module is compiled twice (2 diff versions) and linked twice
  • some part of agent beat includes the same module multiple times from multiple beats

@pchila pchila merged commit 05c5957 into elastic:main May 24, 2024
103 of 106 checks passed
@andrewkroh
Copy link
Member

andrewkroh commented May 24, 2024

coreos/go-systemd copied the code that includes this symbol from coreos/pkg. We use both. So now we have a conflict.

The symbol in question is purely test code so it should have been included in a file with a _test.go suffix.

Haven't had any coffee yet so double check my work 😄 .

fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request May 24, 2024
fearful-symmetry added a commit that referenced this pull request May 31, 2024
…update to `go-systemd` (#39730)

* Reapply "let the system/process metricset monitor a single PID (#39620)" (#39714)

This reverts commit 05c5957.

* reapply single-pid changes, tinker with go-systemd dep

* fix deps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent Label for the Agent team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants