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

*: fix problems with pkg/gate #6110

Merged
merged 3 commits into from
Feb 8, 2023
Merged

*: fix problems with pkg/gate #6110

merged 3 commits into from
Feb 8, 2023

Conversation

GiedriusS
Copy link
Member

  • Fix bug with duplicate metric registration ([main branch] Query - GoPanic after fix for NoStore API match #6102)
  • Create a gate factory that reuses the same metrics but individually they have separate max limits;
  • Fix metric names so that they would reflect what is being limited; previously the word queries was hardcoded everywhere. I checked dashboards/alerts and it seems like those metrics are not used anywhere.

@GiedriusS
query/querier: reuse same gate
05b632e 
We want to use the same gate everywhere.

Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS
gate: fix up metric name
5369592 
Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS
query/gate: refactor gate
16f392f 
Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS GiedriusS force-pushed the reuse_gate_in_all_queryables branch from 7efade6 to 16f392f Compare February 8, 2023 09:32
@GiedriusS GiedriusS marked this pull request as ready for review February 8, 2023 10:05
fpetkovski
fpetkovski approved these changes Feb 8, 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, this looks good. I wonder why the issue was not caught in any test.

@GiedriusS
Copy link
Member Author

GiedriusS commented Feb 8, 2023 •
edited
Loading

Thanks, this looks good. I wonder why the issue was not caught in any test.

The current code in main continuously wraps the same prometheus.Registerer and in /metrics we get: concurrent_selects_concurrent_selects_concurrent_selects_... and so on. That probably leads to high CPU usage 😄

@GiedriusS GiedriusS merged commit 83d2c14 into thanos-io:main Feb 8, 2023
@GiedriusS GiedriusS deleted the reuse_gate_in_all_queryables branch February 8, 2023 12:55
@GiedriusS GiedriusS mentioned this pull request Feb 8, 2023
Closed
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request Apr 17, 2023
@GiedriusS
*: fix problems with pkg/gate (thanos-io#6110)
a0ac248 
* query/querier: reuse same gate

We want to use the same gate everywhere.

Signed-off-by: Giedrius Statkevičius <[email protected]>

* gate: fix up metric name

Signed-off-by: Giedrius Statkevičius <[email protected]>

* query/gate: refactor gate

Signed-off-by: Giedrius Statkevičius <[email protected]>

---------

Signed-off-by: Giedrius Statkevičius <[email protected]>
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request Apr 17, 2023
@GiedriusS
*: fix problems with pkg/gate (thanos-io#6110)
479d96f 
* query/querier: reuse same gate

We want to use the same gate everywhere.

Signed-off-by: Giedrius Statkevičius <[email protected]>

* gate: fix up metric name

Signed-off-by: Giedrius Statkevičius <[email protected]>

* query/gate: refactor gate

Signed-off-by: Giedrius Statkevičius <[email protected]>

---------

Signed-off-by: Giedrius Statkevičius <[email protected]>
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

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

Successfully merging this pull request may close these issues.
2 participants
@GiedriusS @fpetkovski