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

Proxy: Only generate debug messages in debug mode #6228

Merged
merged 3 commits into from Mar 23, 2023
Merged

Proxy: Only generate debug messages in debug mode #6228

merged 3 commits into from Mar 23, 2023

Conversation

jacobbaungard
Copy link
Contributor

@jacobbaungard jacobbaungard commented Mar 20, 2023 •
edited

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

saswatamcode
saswatamcode previously approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks!

Some small suggestions, but looks good otherwise. Would be great to see a comparable profile!

cmd/thanos/query.go Outdated
@@ -229,7 +229,7 @@ func registerQuery(app *extkingpin.App) {
var storeRateLimits store.SeriesSelectLimits
storeRateLimits.RegisterFlags(cmd)

cmd.Setup(func(g *run.Group, logger log.Logger, reg *prometheus.Registry, tracer opentracing.Tracer, _ <-chan struct{}, _ bool) error {
cmd.Setup(func(g *run.Group, logger log.Logger, reg *prometheus.Registry, tracer opentracing.Tracer, _ <-chan struct{}, debug bool) error {
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep it consistent with other components that use it.

Suggested change
cmd.Setup(func(g *run.Group, logger log.Logger, reg *prometheus.Registry, tracer opentracing.Tracer, _ <-chan struct{}, debug bool) error {
cmd.Setup(func(g *run.Group, logger log.Logger, reg *prometheus.Registry, tracer opentracing.Tracer, _ <-chan struct{}, debugLogging bool) error {

pkg/store/proxy.go Show resolved Hide resolved
pkg/store/proxy.go Outdated
@@ -273,7 +276,9 @@ func (s *ProxyStore) Series(originalRequest *storepb.SeriesRequest, srv storepb.
for _, st := range s.stores() {
// We might be able to skip the store if its meta information indicates it cannot have series matching our query.
if ok, reason := storeMatches(srv.Context(), st, originalRequest.MinTime, originalRequest.MaxTime, matchers...); !ok {
storeDebugMsgs = append(storeDebugMsgs, fmt.Sprintf("store %s filtered out: %v", st, reason))
if s.debug {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be a good idea to do the same for ProxyStore LabelNames and LabelValues methods as well, as they also print debug statements the same way?

pkg/store/proxy.go Outdated
@@ -68,6 +68,7 @@ type Client interface {
// ProxyStore implements the store API that proxies request to all given underlying stores.
type ProxyStore struct {
logger log.Logger
debug bool
Copy link
Member

Choose a reason for hiding this comment

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

We can iterate on this but would be nice to have a builder pattern, with option methods like WithProxyDebugLogging similar to BucketStore. That way we can avoid having to change signature everytime. 🙂

@jacobbaungard
Copy link
Contributor Author

Tried the address all the comments.

The e2e test failures seems to be due to Prometheus' busybox image has been updated - looks like there will be an automatic PR updating that soon.

@jacobbaungard jacobbaungard mentioned this pull request Mar 22, 2023
Merged
2 tasks
@jacobbaungard
Proxy: Only generate debug messages in debug mode
0425274 
In debug mode we log information about which endpoints we skip and
query, including the labels of each endpoint. These debug messages, can
use significant memory, especially in larger setups with larger number
of labelsets.

Previously we would generate the debug messages, even when not in debug
mode. With commit, we only generate the debug messages when debug
logging is enabled.

This fixes: thanos-io#6198

Signed-off-by: Jacob Baungard Hansen <jachanse@redhat.com>
@jacobbaungard
Address review comments
ccdb2c3 
- Consistent variable naming
- Also do conditional debug logging generation in: `LabelNames()` and
  `LabelValues`
- Use builder pattern for additional variables

Signed-off-by: Jacob Baungard Hansen <jachanse@redhat.com>
@jacobbaungard
Make linter happy
ebaf19b 
Signed-off-by: Jacob Baungard Hansen <jachanse@redhat.com>
saswatamcode
saswatamcode approved these changes Mar 23, 2023
View reviewed changes
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

LGTM!

fpetkovski
fpetkovski approved these changes Mar 23, 2023
View reviewed changes
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this, LGTM

@fpetkovski fpetkovski merged commit 62ceb82 into thanos-io:main Mar 23, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpetkovski fpetkovski fpetkovski approved these changes

@saswatamcode saswatamcode saswatamcode approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proxy: memory bloat caused by fmt.Sprintf
3 participants
@jacobbaungard @fpetkovski @saswatamcode