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

Remove duplicated loki_boltdb_shipper prefix from tables_upload_operation_total metric #7040

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

bakunowski
Copy link
Contributor

What this PR does / why we need it:
This PR removes the duplicated loki_boltdb_shipper prefix from the tables_upload_operation_total metric.

In #6278 metrics for tracking indexshipper operations were added. WrapRegistererWithPrefix was used to make sure the same metrics as before were exposed (see here).

While the downloads metrics had the Namespace field removed (see here), the uploads metric did not (see here). This results in the tables_upload_operation_total being exposed as loki_boltdb_shipper_loki_boltdb_shipper_tables_upload_operation_total instead of loki_boltdb_shipper_tables_upload_operation_total.

Which issue(s) this PR fixes:
This PR serves both as an issue and a fix.

Special notes for your reviewer:
This is my best interpretation why the exposed name of the uploads metric has changed. If this is incorrect, or the change was intentional - I'm sorry for the noise.

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

@bakunowski bakunowski requested a review from a team as a code owner September 2, 2022 23:40
@CLAassistant
Copy link

CLAassistant commented Sep 2, 2022

CLA assistant check
All committers have signed the CLA.

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Collaborator

@kavirajk kavirajk 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 for the PR @bakunowski. Sounds totally reasonable. Also double checked with metrics coming from internal Loki clusters.

Since this is the user visible change, can you please add it in CHANGELOG as well please? I will merge after that.

@bakunowski
Copy link
Contributor Author

Thanks for the review @kavirajk, glad to hear I wasn't completely off the mark. Updated CHANGELOG in the Fixes section.

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Contributor

@sandeepsukhani sandeepsukhani 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 for fixing it!

@sandeepsukhani sandeepsukhani merged commit 4e7e51c into grafana:main Sep 14, 2022
lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022
…eration_total` metric (grafana#7040)

This PR removes the duplicated `loki_boltdb_shipper` prefix from the
`tables_upload_operation_total` metric.
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.

None yet

5 participants