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

The httpProxy option isn't used by the diagnose transmitter or Push API key validator #518

Closed
1 of 6 tasks
tombruijn opened this issue Nov 25, 2021 · 3 comments
Closed
1 of 6 tasks
Assignees
Labels

Comments

@tombruijn
Copy link
Member

tombruijn commented Nov 25, 2021

Part of https://github.com/appsignal/integration-guide/pull/105

The httpProxy config option is currently only configured to be used by the agent, but the diagnose report transmitter and Push API key validator do not use this config option's value.

We're running into issues using the http/https libraries in Node.js directly do to missing documentation and missing safety checks. Implementing proxy support has been a big chore so far that we haven't been able to figure out.

We need this http(s) capability for the following scenarios:

  • Download the agent and extension in the installer
  • Validate the Push API key on install and diagnose
  • Transmit the diagnose report

Alternative libraries

There's a popular library axios that we can use instead that has more checks and proper proxy support.

We prefer not to add additional dependencies unless absolutely necessary. We've done something similar in Elixir; we use hackney as an HTTP library. Ideally though, we find a way to use the standard library for these types of things.

Research

  • Research if we can improve our usage of the http/https libraries
  • If not, research axios, or other light weight alternatives. Write up a comparison.
  • Decide with the team if we want to use an extra library for http/https.

To do

@tombruijn tombruijn added the bug label Nov 25, 2021
@tombruijn tombruijn changed the title The httpProxy option isn't used by the diagnose transmitter or Push API key The httpProxy option isn't used by the diagnose transmitter or Push API key validator Nov 29, 2021
@luismiramirez luismiramirez self-assigned this Dec 2, 2021
@luismiramirez
Copy link
Member

luismiramirez commented Dec 10, 2021

After working on this one for a while, we decided not to continue as it requires more work than expected.

Node.js native proxy connection support is poorly documented, not to say not documented at all. The best I could find was this StackOverflow answer, and it only works for HTTP. The support is even worse to connect to an HTTPS through a proxy.

We need to spend a bit of research on this one and make a decision as a team:

  • Keep fighting with http and https modules
  • Use an external package that provides proxy support out of the box

The abstraction of transmitters and the certificate configuration support is on the way.

@tombruijn
Copy link
Member Author

As discussed, we can also hold off on working on this for a while. No one has asked for it yet, so I see no rush to put it in if it's a lot of work right now

@unflxw
Copy link
Contributor

unflxw commented Feb 7, 2022

Regarding axios, here's how they implement proxy support themselves: https://github.com/axios/axios/blob/73e3bdb8835ba942096b662e9441f1d85ce4d484/lib/adapters/http.js#L27-L44

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

No branches or pull requests

3 participants