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

[coordinator] only log stack traces on panic #1480

Merged
merged 2 commits into from
Mar 20, 2019

Conversation

schallert
Copy link
Collaborator

It's often clear where errors are coming from. When stack traces are
enabled, if a coordinator is being used as a prom backend and there's an
error with the M3DB cluster then every single prom write causes a stack
trace and the logs quickly fill up, making debugging difficult.

It's often clear where errors are coming from. When stack traces are
enabled, if a coordinator is being used as a prom backend and there's an
error with the M3DB cluster then every single prom write causes a stack
trace and the logs quickly fill up, making debugging difficult.
Copy link
Collaborator

@prateek prateek left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@benraskin92 benraskin92 left a comment

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #1480 into master will decrease coverage by <.1%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1480     +/-   ##
========================================
- Coverage    70.9%   70.8%   -0.1%     
========================================
  Files         841     841             
  Lines       71909   71909             
========================================
- Hits        50988   50962     -26     
- Misses      17574   17595     +21     
- Partials     3347    3352      +5
Flag Coverage Δ
#aggregator 82.3% <ø> (ø) ⬆️
#cluster 85.8% <ø> (ø) ⬆️
#collector 63.7% <ø> (ø) ⬆️
#dbnode 80.6% <ø> (-0.1%) ⬇️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.2% <ø> (ø) ⬆️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.5% <ø> (ø) ⬆️
#msg 74.9% <ø> (ø) ⬆️
#query 66% <100%> (ø) ⬆️
#x 76.3% <ø> (ø) ⬆️

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 69fffb9...010ad86. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #1480 into master will decrease coverage by <.1%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1480     +/-   ##
========================================
- Coverage    70.9%   70.8%   -0.1%     
========================================
  Files         841     841             
  Lines       71909   71909             
========================================
- Hits        50988   50962     -26     
- Misses      17574   17595     +21     
- Partials     3347    3352      +5
Flag Coverage Δ
#aggregator 82.3% <ø> (ø) ⬆️
#cluster 85.8% <ø> (ø) ⬆️
#collector 63.7% <ø> (ø) ⬆️
#dbnode 80.6% <ø> (-0.1%) ⬇️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.2% <ø> (ø) ⬆️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.5% <ø> (ø) ⬆️
#msg 74.9% <ø> (ø) ⬆️
#query 66% <100%> (ø) ⬆️
#x 76.3% <ø> (ø) ⬆️

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 69fffb9...8c9ffe9. Read the comment docs.

Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

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

Hey, not sure if this is necessarily correct, we've had issues debugging open source problems in the past because we couldn't get a full stack trace. If necessary, could we put this behind a flag?

Copy link
Collaborator Author

@schallert schallert left a comment

Choose a reason for hiding this comment

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

LGTM

@schallert schallert changed the title [coordinator] disable stack traces in logs [coordinator] only log stack traces on panic Mar 20, 2019
@schallert schallert merged commit c7aeefd into master Mar 20, 2019
@schallert schallert deleted the schallert/no_coord_stack branch March 20, 2019 19:32
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.

4 participants