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

chore(query-scheduler) Remove nosiy "inflight" requests log entries #6074

Merged
merged 1 commit into from
May 4, 2022

Conversation

kavirajk
Copy link
Contributor

@kavirajk kavirajk commented May 2, 2022

What this PR does / why we need it:
We have enough metrics now (queue time, queue length and metrics.go statistics)
that makes these log entries obselete.

Signed-off-by: Kaviraj kavirajkanagaraj@gmail.com

Which issue(s) this PR fixes:
Fixes #NA

Special notes for your reviewer:
Quickly checked internal and external mixins to verify it's not used in any dashboards or alerts.

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

We have enough metrics now (queue time, queue length and metrics.go statistics)
that makes these log entries obselete.

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@kavirajk kavirajk requested a review from a team as a code owner May 2, 2022 14:38
@kavirajk kavirajk requested a review from slim-bean May 2, 2022 14:38
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Do we have a metric expressing query processing time on a per-tenant basis?

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kavirajk
Copy link
Contributor Author

kavirajk commented May 3, 2022

Do we have a metric expressing query processing time on a per-tenant basis?

No. not per tenant basis. ( I tried once, but reverted because of cardinality explosion)

The reason to remove is mainly it's noisy (two log entries per every request) and also we pay quite a bit for this (heard it from @slim-bean).

@dannykopping
Copy link
Contributor

Do we have a metric expressing query processing time on a per-tenant basis?

No. not per tenant basis. ( I tried once, but reverted because of cardinality explosion)

The reason to remove is mainly it's noisy (two log entries per every request) and also we pay quite a bit for this (heard it from @slim-bean).

Cool, I'm happy to defer to Ed's LGTM here.
Thanks for the clarification.

@kavirajk kavirajk merged commit 0a4bfbe into main May 4, 2022
@kavirajk kavirajk deleted the kavirajk/remove-noisy-infligh-request-log branch May 4, 2022 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants