grafana / pyroscope-java

pyroscope java integration
Apache License 2.0
72 stars 31 forks source link

adding config parameter for no-proxy #146

Closed raflFaisal closed 3 months ago

raflFaisal commented 3 months ago

We recently started exploring pyroscope for profiling our applications, we encountered an issue with establishing connection requests to the Pyroscope server within our organization's network.

Upon starting a Java application, we encountered errors related to this connectivity problem

2024-03-19 12:42:08.851 [ERROR] Error uploading snapshot: timeout 2024-03-19 12:42:19.765 [ERROR] Error uploading snapshot: timeout

This is due to usage of proxy configurations, when we disable the proxy usage and build pyroscope.jar locally, it solves the problem. So we opened a pull-request by adding an optional parameter to enable NO_PROXY option

New introduced configuration parameter : PYROSCOPE_NO_PROXY

false -> no_proxy is disabled (Default functionality) true -> no_proxy is enabled for okHttpClient

CLAassistant commented 3 months ago

CLA assistant check
All committers have signed the CLA.

raflFaisal commented 3 months ago

I have signed the Contributor License Agreement, please let me know if anything else is required from my side.

bryanhuhta commented 3 months ago

@raflFaisal You will need to sign the CLA with @faisalBooking, since that is the account that made the commit. Once that is done I will approve!

faisalBooking commented 3 months ago

Thanks @bryanhuhta for reviewing this change 👍 I have signed the CLA from my official account. Let me know if anything else is required from my side.

faisalBooking commented 3 months ago

Thanks for the review @kolesnikovae

@korniltsev please approve if the code change looks good to you :)

aleks-p commented 3 months ago

Just to understand a bit more about the proxy configuration you are using, have you tried setting the NO_PROXY environment variable (or the Java system property -Dhttp.nonProxyHosts=<host>) when running the application to achieve the same?

faisalBooking commented 3 months ago

@aleks-p Thanks for the suggestion, I was not aware of nonProxyHosts property and it works fine in our case, we were looking for a more granular approach to control this during the connection request.

nonProxyHosts seems suitable for us so I guess we can close the pull-request

aleks-p commented 3 months ago

I am glad that worked. Both the system property and the env variable (NO_PROXY=<pyroscope-host>,<another-host>,...) should in theory provide the same per-application and per-host granularity.

As for the PR, I think we can close it for now and revisit the topic if it becomes relevant in the future.