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

Add redacted addr to scrapeRedisHost error message (#638) #643

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

neomantra
Copy link
Contributor

When scrapeRedisHost fails to connect to a Redis instance, the code now tries to redact the password of the Redis address (URL). If it can parse and redact it, it will print the address in the error message. Otherwise, we note that -debug is needed to see the address. It does not log any URL parsing errors, just in case that may leak any info.

Testing:

> ./redis_exporter  -redis.addr "redis://user:peace@foobar.com:9999"
INFO[0000] Redis Metrics Exporter <<< filled in by build >>>    build date: <<< filled in by build >>>    sha1: <<< filled in by build >>>    Go: go1.18.1    GOOS: darwin    GOARCH: arm64 
INFO[0000] Providing metrics at :9121/metrics           
ERRO[0018] Couldn't connect to redis instance (redis://user:xxxxx@foobar.com:9999)

@coveralls
Copy link

coveralls commented May 14, 2022

Pull Request Test Coverage Report for Build 1560

  • 6 of 8 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 92.037%

Changes Missing Coverage Covered Lines Changed/Added Lines %
exporter/exporter.go 6 8 75.0%
Totals Coverage Status
Change from base Build 1559: -0.08%
Covered Lines: 1780
Relevant Lines: 1934

💛 - Coveralls

@codecov
Copy link

codecov bot commented May 14, 2022

Codecov Report

Merging #643 (5fc1fe5) into master (3afacfe) will decrease coverage by 0.17%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
- Coverage   88.24%   88.07%   -0.18%     
==========================================
  Files          16       16              
  Lines        1922     1928       +6     
==========================================
+ Hits         1696     1698       +2     
- Misses        152      154       +2     
- Partials       74       76       +2     
Impacted Files Coverage Δ
exporter/exporter.go 91.48% <50.00%> (-0.69%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Owner

@oliver006 oliver006 left a comment

Choose a reason for hiding this comment

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

Looks good, just one comment

@@ -527,7 +528,13 @@ func (e *Exporter) scrapeRedisHost(ch chan<- prometheus.Metric) error {
e.registerConstMetricGauge(ch, "exporter_last_scrape_connect_time_seconds", connectTookSeconds)

if err != nil {
log.Errorf("Couldn't connect to redis instance")
var redactedAddr string
if redisURL, _ := url.Parse(e.redisAddr); redisURL != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe catch the error here instead of checking the result for nil? You could then output the error in the else block in log level debug to add addtl information.

@oliver006
Copy link
Owner

@neomantra - do you still plan to make changes on this? I think surfacing the error would be good.

@neomantra
Copy link
Contributor Author

I’ll incorporate that feedback today, thanks.

@neomantra
Copy link
Contributor Author

I updated this per your comment and rebased to latest master.

Copy link
Owner

@oliver006 oliver006 left a comment

Choose a reason for hiding this comment

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

🎉

@oliver006 oliver006 merged commit 07709eb into oliver006:master Jun 3, 2022
@oliver006
Copy link
Owner

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.

3 participants