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

SNOW-926008 supportability/observability enhancement for generic and proxy use-cases #699

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

sfc-gh-dszmolka
Copy link
Collaborator

  • 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

  • proxy: detecting if HTTP_PROXY/HTTPS_PROXY environmental variable is set, log them on DEBUG loglevel

  • proxy: added logging for Connection's proxyPort and proxyUser (if set), had only proxyHost.
    -- this bit of logging now moved to generic agent

  • proxy: if both HTTPS_PROXY and proxyHost is set, can lead to not-so-trivial-to-debug situations ; log this on WARN loglevel for more intuitive troubleshooting

…proxy use-cases

- 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

- proxy: detecting if HTTP_PROXY/HTTPS_PROXY environmental variable is set, log them on DEBUG loglevel
- proxy: added logging for Connection's proxyPort and proxyUser (if set), had only proxyHost. 
-- this bit of logging now moved to generic agent
- proxy: if both HTTPS_PROXY and proxyHost is set, can lead to not-so-trivial-to-debug situations ; log this on WARN loglevel for more intuitive troubleshooting
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (49c7b13) 87.51% compared to head (2fde493) 87.40%.

Files Patch % Lines
lib/agent/https_proxy_ocsp_agent.js 0.00% 2 Missing ⚠️
lib/http/node.js 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
- Coverage   87.51%   87.40%   -0.11%     
==========================================
  Files          61       61              
  Lines        5759     5789      +30     
==========================================
+ Hits         5040     5060      +20     
- Misses        719      729      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/http/node.js Outdated Show resolved Hide resolved
…e the env PROXY with the agent proxy settings and generate log messages accordingly

* moved the proxy (env+agent) detection functionality into agent creation step to try and check it only once, upon agent creation time
* adjusted unit tests accordingly
Copy link
Collaborator

@sfc-gh-pmotacki sfc-gh-pmotacki left a comment

Choose a reason for hiding this comment

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

LGTM

@sfc-gh-dszmolka sfc-gh-dszmolka merged commit 0f58cec into master Nov 16, 2023
43 of 44 checks passed
@sfc-gh-dszmolka sfc-gh-dszmolka deleted the SNOW-926008-observability-for-proxy-usecases branch November 16, 2023 17:24
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants