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

feat: add stackDepthMax builtin opt #98

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

tomershafir
Copy link
Contributor

No description provided.

Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

LGTM

I wonder if in the future (not this PR) we should just allow passing arbitrary params/commsnds to the AP, so that there's less restrictions from the pyroscope-java

@korniltsev korniltsev merged commit 0ef9395 into grafana:main Jul 13, 2023
13 checks passed
@tomershafir tomershafir deleted the feat/add-stack-depth-max-opt2 branch July 14, 2023 06:18
@tomershafir
Copy link
Contributor Author

tomershafir commented Jul 14, 2023

@korniltsev I think you need to make a decision:

  1. Make pyroscope-java tightly coupled to ap, which makes sense because it's a great oss jvm profiler, and an alternative would be hard to find. Then, a single text param will make sense and be flexible.
  2. Make pyroscope-java a simple, loosely coupled abstraction, with profiler agnostic interface. In this case I would even suggest to remove APExtraArgs from the generic interface, and extend it when needed.

I would go with 2.

@korniltsev
Copy link
Collaborator

Oh i forgot we already have APExtraArgs.

I am leaning towards 1 as you can see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants