feat: configuration for custom temporary file paths - #340
Conversation
|
|
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| private static final String PYROSCOPE_HTTP_HEADERS = "PYROSCOPE_HTTP_HEADERS"; | ||
| private static final String PYROSCOPE_TENANT_ID = "PYROSCOPE_TENANT_ID"; | ||
| private static final String PYROSCOPE_PROFILE_EXPORT_TIMEOUT = "PYROSCOPE_PROFILE_EXPORT_TIMEOUT"; | ||
| private static final String PYROSCOPE_JFR_DIR = "PYROSCOPE_JFR_DIR"; |
There was a problem hiding this comment.
Should this be called PYROSCOPE_TMP_DIR ? since it is used for both JFR and jar files
3e9d69d to
c7ea30a
Compare
d9319b7 to
4be8edd
Compare
|
@korniltsev-grafanista let me know how this looks |
korniltsev-grafanista
changed the title
| private String basicAuthUser; | ||
| private String basicAuthPassword; | ||
| private String jfrProfilerSettings; | ||
| private String tmpDir; |
There was a problem hiding this comment.
please annotate tmp dir fields and args as Nullable where appropriate. I know its not done for all the fields, but I think it improves readability
| this.format = config.format; | ||
|
|
||
| if (format == Format.JFR && null == tempJFRFile) { | ||
| if (config.tmpDir != null && config.tmpDir.contains(",")) { |
There was a problem hiding this comment.
move this validation to config builder routines
korniltsev-grafanista
left a comment
korniltsev-grafanista
left a comment
There was a problem hiding this comment.
lgtm with few nits
thanks
Signed commits reportAll 1 commit between |
cc7ecd6 to
4b71396
Compare
Sorry for delay, addressed and push. lmk if all good. |
korniltsev-grafanista
left a comment
korniltsev-grafanista
left a comment
There was a problem hiding this comment.
please take a look at #340 (comment) report, it looks valid to me.
otherwise lgtm
4b71396 to
94bf6d6
Compare
Updated addressing this, with a test too. |
| try { | ||
| resolved = Paths.get(tmpDir).toAbsolutePath().toString(); | ||
| } catch (java.nio.file.InvalidPathException e) { | ||
| resolved = tmpDir; // invalid path is caught later in TmpFileUtil |
There was a problem hiding this comment.
I dont understand, why do we swallow it here and resurface somewhere else?
There was a problem hiding this comment.
done, fixed when Path
| public final String APExtraArguments; | ||
| public final String basicAuthUser; | ||
| public final String basicAuthPassword; | ||
| public final @Nullable String tmpDir; |
There was a problem hiding this comment.
can we make it Path ?
| return createTempFile(config.tmpDir, "pyroscope", ".jfr"); | ||
| } | ||
|
|
||
| public static File createTempFile(String dir, String prefix, String suffix) throws IOException { |
94bf6d6 to
3591c20
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3591c20. Configure here.
3591c20 to
cad5b10
Compare
korniltsev-grafanista
left a comment
korniltsev-grafanista
left a comment
There was a problem hiding this comment.
LGTM
Last request. All contributions are required to sign their commits with gpg/ssh
https://community.grafana.com/t/action-required-signed-commits-mandatory-for-all-grafana-repositories/163404
I can't merge without this
1a87637 to
d501ac8
Compare
…ncludes bootstream jar, and the .jfr files created by the agent delegates. Signed commit
d501ac8 to
f6dbc2c
Compare
I squashed the commits and sorted out the GPG key, commit is showing verified now! |
2 participants
Support config driven locations for .jfr files and bootstrap jars to avoid them going into the /tmp dir for apps that want to control this.