-
Notifications
You must be signed in to change notification settings - Fork 453
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
[query] Log at debug level resolved cluster namespaces, use same logger in query and coordinator #1775
[query] Log at debug level resolved cluster namespaces, use same logger in query and coordinator #1775
Conversation
…m3db/m3 into r/debug-logs-resolve-cluster-namespaces
It works great. Example output in debug mode (it shows two namespaces depending on the retention value): Thx! |
One detail. I put the mode back to |
@vjtm interesting, this may be due to the fact two loggers exist when coordinator and dbnode run side by side. I might fix that too with this PR. |
Updated with all loggers being constructed equally now in the same process, that should avoid what you're seeing with the logs being logged regardless of config. |
Hi, I have the following in place (just for testing):
The m3dbnode1 with debug mode activated show debug messages in the logs, but no The cluster works fine. I see data in grafana. Metrics are comming from https://github.com/open-fresh/avalanche I am @vjtm, just using a different account :) |
Hey @vtomasr5 @vjtm the reason I believe you're not seeing the message anymore is you actually need to set log level to debug for the coordinator specifically in your config file. So at the top of logging:
level: debug I manage to see the correct debug output for the namespaces resolved when sending queries to the coordinator itself now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved /w nits; only real point is we may miss visibility in local v remote storage
SetLookbackDuration(h.lookbackDuration). | ||
SetGlobalEnforcer(nil). | ||
SetInstrumentOptions(h.instrumentOpts. | ||
SetMetricsScope(h.instrumentOpts.MetricsScope().SubScope("debug_engine"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: split this up into 2 statements?
} | ||
|
||
// NewOptions enforces that fields are set when options is created. | ||
func NewOptions(p OptionsParams) (Options, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for, just removing direct struct generation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was possible to create it without InstrumentOptions
and consequently it could easily lead to nil pointer panics. So now it's protected against that by always enforcing it to be set when constructing Options
.
SetStore(backendStorage). | ||
SetLookbackDuration(*cfg.LookbackDuration). | ||
SetGlobalEnforcer(perQueryEnforcer). | ||
SetInstrumentOptions(instrumentOptions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: split into 2 statements
debugLog := s.logger.Check(zapcore.DebugLevel, | ||
"query resolved cluster namespace, will use most granular per result") | ||
if debugLog != nil { | ||
for _, n := range namespaces { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick note: currently I'm not sure you'll be able to tell if you're serving the query yourself or serving a remote query here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably ok for just simple debugging. We can get more complex later perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's fine; only concern is if you get random spam in your logs from remote queries
src/query/tsdb/remote/codecs.go
Outdated
@@ -241,7 +245,7 @@ func retrieveMetadata(streamCtx context.Context) context.Context { | |||
} | |||
} | |||
|
|||
return logging.NewContextWithID(streamCtx, id) | |||
return logging.NewContextWithID(streamCtx, instrumentOpts, id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: put options last here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
src/query/util/logging/log.go
Outdated
// Attach a rqID with all logs so that its simple to trace the whole call stack | ||
rqID := uuid.NewRandom() | ||
return NewContextWithID(ctx, rqID.String()) | ||
return NewContextWithID(ctx, instrumentOpts, rqID.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: options last here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
src/query/util/logging/log.go
Outdated
func NewContextWithID(ctx context.Context, id string) context.Context { | ||
func NewContextWithID( | ||
ctx context.Context, | ||
instrumentOpts instrument.Options, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: set the options as last param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
I've added those lines at the top of But I am unable to see the debug logging.
On the other hand, in the |
@vtomasr5 are you sure you are sending queries to that coordinator? I'm seeing it in my tests:
Relevant build details:
|
I'm sorry. Prometheus was not starting fine. It works perfect. |
What this PR does / why we need it:
A few people have requested at least some basic insight to what namespaces are being resolved, this is adding the first level of debugging in this area. (More to come later, this we can get out quickly).
Also I needed to flow the config to control the logging level through to the query/coordinator so I ended up consolidating all our logging so that it's the exact same loggers using instrument options all the way down in query and coordinator.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: