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.

Sign up for GitHub

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

Jump to bottom

[coordinator] Remove carbon debug flag and rely on log debug level #2024

Merged
merged 8 commits into from
Oct 29, 2019
Merged

[coordinator] Remove carbon debug flag and rely on log debug level #2024

merged 8 commits into from
Oct 29, 2019

Conversation

robskillington
Copy link
Collaborator

What this PR does / why we need it:

Fixes having two configs for controlling debug/non-debug messages for carbon ingester. Now just use the debug log level to determine whether to print debug messages for the carbon ingester.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

arnikola
arnikola reviewed Oct 28, 2019
View reviewed changes
src/cmd/services/m3coordinator/ingest/carbon/ingest.go Outdated
@@ -30,6 +30,8 @@ import (
"sync"
"time"

"go.uber.org/zap/zapcore"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: wrong segment for this import

arnikola
arnikola approved these changes Oct 28, 2019
View reviewed changes
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.

Approved /w nit

@robskillington
Merge branch 'r/remove-carbon-debug-flags-rely-on-debug-level-from-lo…
360cd5f 
…gger' of github.com:m3db/m3 into r/remove-carbon-debug-flags-rely-on-debug-level-from-logger
@robskillington
Use checked entries everywhere
d310787
@robskillington robskillington added the ci Triggers CI (useful for external contributors) label Oct 28, 2019
@schallert schallert removed the ci Triggers CI (useful for external contributors) label Oct 28, 2019
@mmedvede
Copy link
Contributor

Would this break an existing deployments that set this option?

@codecov
Copy link

codecov bot commented Oct 28, 2019 •
edited
Loading

Codecov Report

Merging #2024 into master will decrease coverage by 10.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2024      +/-   ##
=========================================
- Coverage    72.4%   62.2%   -10.2%     
=========================================
  Files        1007     971      -36     
  Lines       86645   84256    -2389     
=========================================
- Hits        62753   52439   -10314     
- Misses      19657   28409    +8752     
+ Partials     4235    3408     -827
Flag Coverage Δ
#aggregator 71.5% <ø> (-4.1%) ⬇️
#cluster 39.4% <ø> (-43.4%) ⬇️
#collector 64.3% <ø> (+0.9%) ⬆️
#dbnode 73.1% <ø> (+3.3%) ⬆️
#m3em 64.6% <ø> (-8.6%) ⬇️
#m3ninx 71.5% <ø> (+44.3%) ⬆️
#m3nsch 30.7% <ø> (-20.5%) ⬇️
#metrics 30.2% <ø> (+12.5%) ⬆️
#msg 74.9% <ø> (+0.6%) ⬆️
#query 44.6% <ø> (-25.6%) ⬇️
#x 55.8% <ø> (-24.8%) ⬇️

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 83ccc30...d310787. Read the comment docs.

4 similar comments
@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #2024 into master will decrease coverage by 10.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2024      +/-   ##
=========================================
- Coverage    72.4%   62.2%   -10.2%     
=========================================
  Files        1007     971      -36     
  Lines       86645   84256    -2389     
=========================================
- Hits        62753   52439   -10314     
- Misses      19657   28409    +8752     
+ Partials     4235    3408     -827
Flag Coverage Δ
#aggregator 71.5% <ø> (-4.1%) ⬇️
#cluster 39.4% <ø> (-43.4%) ⬇️
#collector 64.3% <ø> (+0.9%) ⬆️
#dbnode 73.1% <ø> (+3.3%) ⬆️
#m3em 64.6% <ø> (-8.6%) ⬇️
#m3ninx 71.5% <ø> (+44.3%) ⬆️
#m3nsch 30.7% <ø> (-20.5%) ⬇️
#metrics 30.2% <ø> (+12.5%) ⬆️
#msg 74.9% <ø> (+0.6%) ⬆️
#query 44.6% <ø> (-25.6%) ⬇️
#x 55.8% <ø> (-24.8%) ⬇️

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 83ccc30...d310787. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #2024 into master will decrease coverage by 10.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2024      +/-   ##
=========================================
- Coverage    72.4%   62.2%   -10.2%     
=========================================
  Files        1007     971      -36     
  Lines       86645   84256    -2389     
=========================================
- Hits        62753   52439   -10314     
- Misses      19657   28409    +8752     
+ Partials     4235    3408     -827
Flag Coverage Δ
#aggregator 71.5% <ø> (-4.1%) ⬇️
#cluster 39.4% <ø> (-43.4%) ⬇️
#collector 64.3% <ø> (+0.9%) ⬆️
#dbnode 73.1% <ø> (+3.3%) ⬆️
#m3em 64.6% <ø> (-8.6%) ⬇️
#m3ninx 71.5% <ø> (+44.3%) ⬆️
#m3nsch 30.7% <ø> (-20.5%) ⬇️
#metrics 30.2% <ø> (+12.5%) ⬆️
#msg 74.9% <ø> (+0.6%) ⬆️
#query 44.6% <ø> (-25.6%) ⬇️
#x 55.8% <ø> (-24.8%) ⬇️

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 83ccc30...d310787. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 28, 2019 •
edited
Loading

Codecov Report

Merging #2024 into master will decrease coverage by 10.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2024      +/-   ##
=========================================
- Coverage    72.4%   62.2%   -10.2%     
=========================================
  Files        1007     971      -36     
  Lines       86645   84256    -2389     
=========================================
- Hits        62753   52439   -10314     
- Misses      19657   28409    +8752     
+ Partials     4235    3408     -827
Flag Coverage Δ
#aggregator 71.5% <ø> (-4.1%) ⬇️
#cluster 39.4% <ø> (-43.4%) ⬇️
#collector 64.3% <ø> (+0.9%) ⬆️
#dbnode 73.1% <ø> (+3.3%) ⬆️
#m3em 64.6% <ø> (-8.6%) ⬇️
#m3ninx 71.5% <ø> (+44.3%) ⬆️
#m3nsch 30.7% <ø> (-20.5%) ⬇️
#metrics 30.2% <ø> (+12.5%) ⬆️
#msg 74.9% <ø> (+0.6%) ⬆️
#query 44.6% <ø> (-25.6%) ⬇️
#x 55.8% <ø> (-24.8%) ⬇️

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 83ccc30...d310787. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #2024 into master will decrease coverage by 10.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2024      +/-   ##
=========================================
- Coverage    72.4%   62.2%   -10.2%     
=========================================
  Files        1007     971      -36     
  Lines       86645   84256    -2389     
=========================================
- Hits        62753   52439   -10314     
- Misses      19657   28409    +8752     
+ Partials     4235    3408     -827
Flag Coverage Δ
#aggregator 71.5% <ø> (-4.1%) ⬇️
#cluster 39.4% <ø> (-43.4%) ⬇️
#collector 64.3% <ø> (+0.9%) ⬆️
#dbnode 73.1% <ø> (+3.3%) ⬆️
#m3em 64.6% <ø> (-8.6%) ⬇️
#m3ninx 71.5% <ø> (+44.3%) ⬆️
#m3nsch 30.7% <ø> (-20.5%) ⬇️
#metrics 30.2% <ø> (+12.5%) ⬆️
#msg 74.9% <ø> (+0.6%) ⬆️
#query 44.6% <ø> (-25.6%) ⬇️
#x 55.8% <ø> (-24.8%) ⬇️

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 83ccc30...d310787. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 28, 2019 •
edited
Loading

Codecov Report

Merging #2024 into master will increase coverage by 0.8%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2024     +/-   ##
========================================
+ Coverage    71.9%   72.7%   +0.8%     
========================================
  Files        1004    1004             
  Lines       86566   86445    -121     
========================================
+ Hits        62250   62921    +671     
+ Misses      20095   19306    -789     
+ Partials     4221    4218      -3
Flag Coverage Δ
#aggregator 52.1% <0%> (-28.3%) ⬇️
#cluster 48.8% <0%> (-5.3%) ⬇️
#collector 47.5% <0%> (-25.1%) ⬇️
#dbnode 78.8% <0%> (+24.1%) ⬆️
#m3em 49.8% <0%> (-10.3%) ⬇️
#m3ninx 64.5% <0%> (-13.8%) ⬇️
#m3nsch 74.7% <0%> (+46.9%) ⬆️
#metrics 53.5% <0%> (+35.8%) ⬆️
#msg 58% <0%> (-4.8%) ⬇️
#query 40% <0%> (-13.7%) ⬇️
#x 55.8% <0%> (-14.9%) ⬇️

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 a08f268...747ee09. Read the comment docs.

arnikola
arnikola reviewed Oct 28, 2019
View reviewed changes
src/cmd/services/m3coordinator/ingest/carbon/ingest.go
@@ -67,7 +68,6 @@ var (

// Options configures the ingester.
type Options struct {
Debug bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw may be prudent to put a deprecation warning here (unfortunately don't think we can delete the field without yaml parser complaining...)

@robskillington robskillington merged commit 7da4640 into master Oct 29, 2019
@robskillington robskillington deleted the r/remove-carbon-debug-flags-rely-on-debug-level-from-logger branch October 29, 2019 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnikola arnikola arnikola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@robskillington @mmedvede @arnikola @schallert