-
Notifications
You must be signed in to change notification settings - Fork 433
pyroscope.java: allow custom asprof distributions instead of embeded one. #4452
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
pyroscope.java: allow custom asprof distributions instead of embeded one. #4452
Conversation
… Profiler(use Distribution).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I left a few minor remarks, none of them blocking.
TmpDir string `alloy:"tmp_dir,attr,optional"` | ||
ProfilingConfig ProfilingConfig `alloy:"profiling_config,block,optional"` | ||
|
||
// undocumented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the plan to document this at a later time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice if there was a notion of experimental configuration. I am just scared to commiting to maintain this. We can document it / make it public separately.
format int | ||
} | ||
|
||
func (a *Archive) SHA1() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make this private?
dist asprof.Distribution | ||
err error | ||
) | ||
if a.Dist != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some logging here to know if we are running with an embedded or a custom distribution?
* pyroscope.java: add integration tests fix package name Revert "pyroscope.java: Fix java log level parameter (#4440)" This reverts commit 4909877. move the helper to pyroscope package * second integration test * revert compose tests * revert unneeded changes * fix buildtag * fix buildtag * improve start time for pyroscope container * skip integration test if it's not pyoroscope job * update makefile * pyroscope.java: Fix java log level parameter (#4440) * pyroscope.java: Fix java log level parameter The version bundled of the async profiler has no loglevel parameter: ``` ts=2025-09-16T08:16:50.898924708Z level=error component_path=/profiling.feature component_id=pyroscope.java.java_pods pid=1184752 err="failed to start: asprof failed to run: asprof failed to run /tmp/alloy-asprof-ae0261b1093f2bc4df44a87300fef98dcdebccb5/bin/asprof: exit status 1 Unrecognized option: --loglevel\n" ``` * Quiet is not a valid argument for the async-profiler cli It can only be used for attaching using agent * remove comments * fix --------- Co-authored-by: Christian Simon <[email protected]>
ExtractDistributions
fromasprof.Profiler
asprof.Profiler
alltogether - after decoupling archive extraction logic, turns out it does nothing - useasprof.Distribution
instead everywhere.asprof
distribution - useful for cases when there is a fix for some problem upstream, but alloy has not been updated yet, so users could have a workaround without waiting for pyroscope to update the asprof within the alloy. Also useful for development - clone asprof, run make, point alloy to the build dir.This is for the next release, we should wait before RC is cut