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

adding config parameter for no-proxy #146

Closed
wants to merge 1 commit into from

Conversation

raflFaisal
Copy link

@raflFaisal raflFaisal commented Mar 19, 2024

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
Copy link

CLAassistant commented Mar 19, 2024

CLA assistant check
All committers have signed the CLA.

@raflFaisal
Copy link
Author

raflFaisal commented Mar 19, 2024

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

@bryanhuhta
Copy link
Contributor

@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
Copy link

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.

Copy link

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you for the contribution @raflFaisal! I'd also like @korniltsev to take a look before merging

@faisalBooking
Copy link

faisalBooking commented Mar 25, 2024

Thanks for the review @kolesnikovae

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

@aleks-p
Copy link
Contributor

aleks-p commented Mar 26, 2024

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
Copy link

@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
Copy link
Contributor

aleks-p commented Mar 28, 2024

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.

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.

6 participants