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.

Sign up for GitHub

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

Jump to bottom

runtime/interpreter: Check the paths first for faster and cheaper detection #2196

Merged
merged 2 commits into from
Nov 2, 2023
Merged

runtime/interpreter: Check the paths first for faster and cheaper detection #2196

merged 2 commits into from
Nov 2, 2023

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Oct 31, 2023 •
edited

@kakkoyun kakkoyun requested a review from a team as a code owner October 31, 2023 20:07
@kakkoyun kakkoyun changed the title Check the paths first for faster and cheaper detection runtime/interpreter: Check the paths first for faster and cheaper detection Oct 31, 2023
@kakkoyun kakkoyun marked this pull request as draft November 1, 2023 08:05
@kakkoyun
Check the paths first for faster and cheaper detection
735d110 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun marked this pull request as ready for review November 1, 2023 19:51
javierhonduco
javierhonduco approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@javierhonduco javierhonduco left a comment

Choose a reason for hiding this comment

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

LGTM! Perhaps I missed it but I could not see Python or Ruby stacks in the profile, could you add them if they aren't present?

pkg/runtime/ruby/ruby.go Outdated Show resolved Hide resolved
pkg/runtime/ruby/ruby.go Outdated Show resolved Hide resolved
pkg/runtime/python/python.go Outdated Show resolved Hide resolved
pkg/runtime/python/python.go Outdated Show resolved Hide resolved
@kakkoyun
Copy link
Member Author

kakkoyun commented Nov 2, 2023

LGTM! Perhaps I missed it but I could not see Python or Ruby stacks in the profile, could you add them if they aren't present?

The profile just from the agent, not the targets.

I can add them as well.

@javierhonduco
Copy link
Contributor

We don't have integration tests for the interpreters, while I don't think this PR introduces any bugs, I think it would be a good idea to verify that everything still works as expected

@kakkoyun
Copy link
Member Author

kakkoyun commented Nov 2, 2023

We don't have integration tests for the interpreters, while I don't think this PR introduces any bugs, I think it would be a good idea to verify that everything still works as expected

Yes, of course, I verified it. I'll attach a profile.

@kakkoyun
Address review issues
f8a5e14 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun merged commit 88e4bb3 into main Nov 2, 2023
22 checks passed
@kakkoyun kakkoyun deleted the interpreter_detection branch November 2, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierhonduco javierhonduco javierhonduco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kakkoyun @javierhonduco