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

Replace fatal error of jaeger initialization with print #2777

Merged
merged 3 commits into from
Oct 6, 2021

Conversation

Gituser143
Copy link
Contributor

@Gituser143 Gituser143 commented Oct 3, 2021

Replace Fatalf() with Println() to log when unable to initialize a tracer. This avoid unnecessary exits. Additionally, total 3 attempts are made to init the tracer.

Fixes #2642

CC: @aeneasr

Checklist

@CLAassistant
Copy link

CLAassistant commented Oct 3, 2021

CLA assistant check
All committers have signed the CLA.

@Gituser143
Copy link
Contributor Author

Hi @aeneasr, I'm not sure why the CI test failed, could you tell me what I'm missing? I wasn't able to make sense of the logs either, the below step failed due to several connections being refused.

.bin/go-acc -o coverage.out ./... -- -failfast -timeout=20m -tags sqlite

driver/registry_base.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Oct 4, 2021

Ah yeah, that's just a flaky test :/

@Gituser143
Copy link
Contributor Author

Gituser143 commented Oct 4, 2021

@aeneasr I've removed the retries and just logged the error as of now. Do let me know if I should revert to the 3 attempts + sleep approach.

PS: Could you please add the hacktoberfest-accepted label to the PR before merging? I would love to have this be one of my contributions :P

@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #2777 (6fa342e) into master (8901b09) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2777      +/-   ##
==========================================
- Coverage   50.42%   50.42%   -0.01%     
==========================================
  Files         235      235              
  Lines       14773    14775       +2     
==========================================
+ Hits         7450     7451       +1     
  Misses       6664     6664              
- Partials      659      660       +1     
Impacted Files Coverage Δ
driver/registry_base.go 88.93% <33.33%> (-0.36%) ⬇️
oauth2/hook.go 64.51% <100.00%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3710468...6fa342e. Read the comment docs.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

driver/registry_base.go Outdated Show resolved Hide resolved
@Gituser143
Copy link
Contributor Author

@aeneasr is there anything else I can add to this PR?

@aeneasr
Copy link
Member

aeneasr commented Oct 5, 2021

Just waiting for CI to pass :) It's flaky atm but I am restarting the job now

@Gituser143
Copy link
Contributor Author

Hey @aeneasr, is there anything I have to change to pass the code coverage CI?

@aeneasr aeneasr merged commit 433ce74 into ory:master Oct 6, 2021
@aeneasr
Copy link
Member

aeneasr commented Oct 6, 2021

thank you :)

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.

Jaeger being unavailable is a fatal error that stops service from starting
3 participants