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

Pass pid to UprobeOptions #742

Merged
merged 9 commits into from
Apr 9, 2024
Merged

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented Apr 1, 2024

This PR passes the PID to UprobeOptions.
The scenario where this makes a difference is when multiple processes use the same executable, we want to make sure we only probe the relevant process.

@RonFed RonFed added the bug Something isn't working label Apr 1, 2024
@RonFed RonFed marked this pull request as ready for review April 1, 2024 13:35
@RonFed RonFed requested a review from a team April 1, 2024 13:35
@damemi
Copy link
Contributor

damemi commented Apr 1, 2024

Thanks @RonFed, do you think it would be possible to add a pid fork in the e2e samples to test this?

also sorry to nag about tests... our infra is lacking and it should be a lot easier than it currently is to test changes like this

@RonFed
Copy link
Contributor Author

RonFed commented Apr 1, 2024

Thanks @RonFed, do you think it would be possible to add a pid fork in the e2e samples to test this?

also sorry to nag about tests... our infra is lacking and it should be a lot easier than it currently is to test changes like this

I'm in favor of improving our tests, however I can't think of a way to test this case without a big refactor to the test infra.
Currently in sample-job.yml we use OTEL_GO_AUTO_TARGET_EXE which should potentially cause the instrumentation of every process running the specified executable. So assuming 2 processes using the same binary the only way to configure the instrumentation to a specific one is passing the InstrumentationOption of WithPID (we don't have an equielent env var for this option as it does not really make sense for env var to contain pid I think).
If you have something in mind that we can do with the current infra to test this case, I'll be happy to add it.

Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This looks good to me.

@damemi
Copy link
Contributor

damemi commented Apr 3, 2024

So assuming 2 processes using the same binary the only way to configure the instrumentation to a specific one is passing the InstrumentationOption of WithPID (we don't have an equielent env var for this option as it does not really make sense for env var to contain pid I think).

Ah, I thought this PR would automatically instrument every process under an executable.

So this really only impacts users that are directly creating an Instrumentation object? (Fwiw, we should expose those options to users of the agent image too)

@MrAlias MrAlias merged commit 1ad37fc into open-telemetry:main Apr 9, 2024
16 checks passed
@damemi damemi mentioned this pull request Apr 10, 2024
@MrAlias MrAlias added this to the v0.12.0-alpha milestone Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants