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

Add panel for inflight object store requests to reads dashboard #2914

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Sep 7, 2022

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

What this PR does

Add a new panel to the store-gateway section of the "Mimir / Reads" dashboard for the total number of inflight object store requests from the store-gateways.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters marked this pull request as ready for review September 7, 2022 19:02
@56quarters
Copy link
Contributor Author

Row screenshot
Screenshot 2022-09-07 at 14-27-13 Mimir _ Reads - Dashboards - Grafana

@replay
Copy link
Contributor

replay commented Sep 7, 2022

I like the idea of seeing the number of inflight requests to the object store, but wouldn't it make more sense to put that on a row about the object store like for example Blocks object store (store-gateway accesses)? If it's on the row about the store-gateway it could easily be misinterpreted as the number of inflight requests to the store-gateway.

@56quarters
Copy link
Contributor Author

56quarters commented Sep 7, 2022

I like the idea of seeing the number of inflight requests to the object store, but wouldn't it make more sense to put that on a row about the object store like for example Blocks object store (store-gateway accesses)? If it's on the row about the store-gateway it could easily be misinterpreted as the number of inflight requests to the store-gateway.

Yeah, I can see that. However, the code that emits this metric is part of a "gate" used to limit requests that is only used in the store-gateway (since the store-gateway is one of the few things doing significant operations on object storage).

If we added this to the object storage section of the dashboard, it would either be missing for the querier object store metrics or we'd have to add this gate to the queriers.

@replay
Copy link
Contributor

replay commented Sep 7, 2022

If we added this to the object storage section of the dashboard, it would either be missing for the querier object store metrics or we'd have to add this gate to the queriers.

I'd just accept the fact that it is missing for the queriers and only add it to the store-gw object store metrics, as this reflects the reality of the metrics that we have. If someone is bothered by this, they could add the equivalent metrics to the queriers in the future. But this is just my opinion and I wouldn't be surprised if others disagree.

@56quarters
Copy link
Contributor Author

If we added this to the object storage section of the dashboard, it would either be missing for the querier object store metrics or we'd have to add this gate to the queriers.

I'd just accept the fact that it is missing for the queriers and only add it to the store-gw object store metrics, as this reflects the reality of the metrics that we have. If someone is bothered by this, they could add the equivalent metrics to the queriers in the future. But this is just my opinion and I wouldn't be surprised if others disagree.

OK, that makes sense to me. I'll go ahead move it there.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Copy link
Contributor Author

New screenshots
Screenshot 2022-09-07 at 17-51-29 Mimir _ Reads - Dashboards - Grafana

Screenshot 2022-09-07 at 17-51-51 Mimir _ Reads - Dashboards - Grafana

Copy link
Contributor

@replay replay 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 addressing my comments,
Looks great!

@56quarters 56quarters merged commit 8abfd85 into main Sep 8, 2022
@56quarters 56quarters deleted the 56quarters/inflight-panel branch September 8, 2022 15:55
@56quarters 56quarters self-assigned this Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants