SNOW-926008 observability enhancement for proxy use-cases #697
Conversation
sfc-gh-dszmolka
commented
Nov 10, 2023
sfc-gh-dszmolka
commented
- detecting if HTTP_PROXY/HTTPS_PROXY environmental variable is set, log them on DEBUG loglevel
- added logging for proxy port and proxy user (if set), had only proxy host
- if both HTTPS_PROXY and proxyHost is set, can lead to not-so-trivial-to-debug situations ; log this on DEBUG loglevel for more intuitive troubleshooting
- generic observability: instead of swallowing the underlying error upon connectivity issue and emit a hard-to-debug generic error message, log that underlying error stack on DEBUG loglevel
* detecting if HTTP_PROXY/HTTPS_PROXY environmental variable is set, log them on DEBUG loglevel * added logging for proxy port and proxy user (if set), had only proxy host * if both HTTPS_PROXY and proxyHost is set, can lead to not-so-trivial-to-debug situations ; log this on DEBUG loglevel for more intuitive troubleshooting * generic observability: instead of swallowing the underlying error upon connectivity issue and emit a hard-to-debug generic error message, log that underlying error stack on DEBUG loglevel
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #697 +/- ##
==========================================
- Coverage 87.35% 87.22% -0.14%
==========================================
Files 61 61
Lines 5759 5785 +26
==========================================
+ Hits 5031 5046 +15
- Misses 728 739 +11 ☔ View full report in Codecov by Sentry. |
lib/agent/https_proxy_ocsp_agent.js
Outdated
| @@ -98,7 +98,25 @@ function connect(req, opts, fn) | |||
| { | |||
| var proxy = this.proxy; | |||
| var agent = this; | |||
| Logger.getInstance().debug("Using proxy=%s for host %s", proxy.host, opts.host); | |||
|
|
|||
| // try and detect if there's proxy envvar set, currently it's not logged anywhere | |||
There was a problem hiding this comment.
do we support env variables for proxy in NodeJs driver?
There was a problem hiding this comment.
we do not, to my knowledge.
that does not keep users from trying to still use it (most of the times inadvertently, e.g. it's already set in the env for other application. or trying to use it since our other drivers support it), then get in all sorts of trouble, ending up with Snowflake Support who now doesn't have a really easy way of confirming this
There was a problem hiding this comment.
should we do anything more than only reading it? eomeone who reads logs will see that maybe there is some difference between those options and will it be enough?
Here we are using agent that is aware of proxy so we can see only differences between driver and env configuration - maybe checking proxy env variables should also be done when we don't have proxy configuration on connection?
There was a problem hiding this comment.
thank you - i'll work on this per our discussion
There was a problem hiding this comment.
re: the env variables: even though it is not documented yet, my tests show that we do support it. I mean the connection breaks as soon as I point HTTP_PROXY / HTTPS_PROXY to a non-existing proxy. It can be confirmed it does go through the proxy as soon as I set up a proxy and set HTTPS_PROXY to point the driver towards it.
Even without proxyHost/proxyPort.
There was a problem hiding this comment.
PROXY envvar now also should be detected outside of the agent
* detect and log PROXY envvar even outside of agent (if proxyHost not used) * detect and log if both the envvar and proxyHost/proxyPort set up * test for the 2 new functions
| // using both the envvar and the proxyHost/proxyPort settings for proxy, this might conflict | ||
| if (envProxy.httpProxy && | ||
| Util.removeScheme(envProxy.httpProxy).toLowerCase() != prxHostWithPort.toLowerCase()) { | ||
| Logger.getInstance().debug('Using both the HTTP_PROXY (%s) and the proxyHost:proxyPort (%s) settings to connect, ' |
There was a problem hiding this comment.
when it's different maybe let's log it with WARN, also in other added logs
There was a problem hiding this comment.
ended up moving this check to somewhere else instead, where we create the agent
also logging the agent-specific proxy details where we create the agent
| @@ -615,6 +615,9 @@ function StateAbstract(options) | |||
| // if we got an error, wrap it into a network error | |||
| if (err) | |||
| { | |||
| // if we're running in DEBUG loglevel, probably we want to see the full error instead | |||
| Logger.getInstance().debug(JSON.stringify(err, Util.getCircularReplacer())) | |||
There was a problem hiding this comment.
could we add a prefixing message at the begining of this log? to not only log the reasult error as json
| @@ -1264,6 +1267,13 @@ StateConnecting.prototype.continue = function () | |||
| // issue a login request | |||
| const sendRequest = function () | |||
| { | |||
| // try to detext PROXY envvar even outside of agent | |||
There was a problem hiding this comment.
we can detect it once when agent is created (agents are reused now) - proxy env variables should not change during application work and it's not necessary to have it on each request?
There was a problem hiding this comment.
good hint, thank you! moved it to where we create the agent
|
closing this PR for now until i clear some test locally |
