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

fix mimir writes resources disk usage dashboard's graphs #502

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

QuantumEnigmaa
Copy link
Contributor

Towards https://github.com/giantswarm/giantswarm/issues/30090

The graphs for Disk reads and Disk writes also display data for the non-PV storage used by the ingesters for the /etc/mimir path but as the graphs are showing the expected data anyway, this "noise" can just be ignored.

Checklist

  • Update changelog in CHANGELOG.md in an end-user friendly language.

@QuantumEnigmaa QuantumEnigmaa self-assigned this Apr 15, 2024
@QuantumEnigmaa QuantumEnigmaa requested a review from a team as a code owner April 15, 2024 10:06
@QuantumEnigmaa
Copy link
Contributor Author

Before :
image

After :
image

@QuantumEnigmaa
Copy link
Contributor Author

Had someone removed this dashboard ? Nothing mentions such a thing in the changelog but this PR makes it appear as if I was adding a brand new file

Copy link
Contributor

@QuentinBisson QuentinBisson left a comment

Choose a reason for hiding this comment

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

Oh it's not in the correct path, the mimir dashboard should be here https://github.com/giantswarm/dashboards/tree/main/helm/dashboards/charts/private_dashboards_mz/dashboards/shared/private not in the al folder

@QuentinBisson
Copy link
Contributor

Can you reformat using vscode and CTRL shift I ?

@QuantumEnigmaa
Copy link
Contributor Author

Can you reformat using vscode and CTRL shift I ?

Well I don't mind but nothing happens :/

@QuentinBisson
Copy link
Contributor

I'll run it tomorrow. I Guess we don't have thé same formatters 😅

Copy link
Contributor

@QuentinBisson QuentinBisson left a comment

Choose a reason for hiding this comment

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

Approving now but let's wait tomorrow

@hervenicol
Copy link
Contributor

Can you reformat using vscode and CTRL shift I ?

Why should we manually change these files? I guess they were exported from Grafana, I'm in favor of keeping them as-is.
If we add some manual things like changes in indentation, we will lose this next time we export them from Grafana.

@QuentinBisson
Copy link
Contributor

Thé thing is we all have custom indentation and reviewing makes it really hard, so we need to standardize

@hervenicol
Copy link
Contributor

Thé thing is we all have custom indentation and reviewing makes it really hard, so we need to standardize

For "handwritten" code I agree, but for generated one like the dashboards I'm don't see the point.

@QuentinBisson
Copy link
Contributor

It's easy, if I edit the expression in the file, my formatter will rewrite everything

@QuentinBisson
Copy link
Contributor

Btw, I think this should be fixed by automation and not me coming to format files :D

@QuentinBisson
Copy link
Contributor

Adding automation here #509

@QuantumEnigmaa
Copy link
Contributor Author

@QuentinBisson @hervenicol are we good to merge here ?

@QuentinBisson
Copy link
Contributor

I'm fine yes

@hervenicol
Copy link
Contributor

I'm fine with these changes, yes.

Didn't find the documentation about what we changed from upstream mixins, so we can apply it back next time we import the Mimir mixins.
If we have it we should probably add a word about these changes.

@hervenicol hervenicol mentioned this pull request Apr 16, 2024
1 task
@QuantumEnigmaa
Copy link
Contributor Author

Didn't find the documentation about what we changed from upstream mixins, so we can apply it back next time we import the Mimir mixins.

If you mean what changed compared to the original upstream mixin dashboard, I changed the cluster labels used in the Disk storage graphs' queries to cluster_id as well as uptading the global cluster parameter for the whole dashboard from label_values(cortex_build_info, cluster) to "abel_values(mimir_build_info, cluster_id)

So I guess it's mainly a custom change from our side which is not relevant for upstream (except for the the cortex to mimir change maybe)

@hervenicol hervenicol mentioned this pull request Apr 16, 2024
1 task
@QuantumEnigmaa QuantumEnigmaa merged commit 0034d01 into main Apr 16, 2024
4 checks passed
@QuantumEnigmaa QuantumEnigmaa deleted the fix-mimir-ingester-disk-graphs branch April 16, 2024 13:10
QuentinBisson added a commit that referenced this pull request Apr 25, 2024
* fix mimir writes resources disk usage dashboard's graphs

* move dashboard to adequate directory

* Reformat file

* merge with main

---------

Co-authored-by: QuentinBisson <[email protected]>
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.

3 participants