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

Include entry metadata bytes in metrics tracking ingestion stats #9808

Merged

Conversation

salvacorts
Copy link
Contributor

What this PR does / why we need it:
In #9694 we added support for adding metadata labels to each entry in a push payload. In this PR we update the following metrics to take into account the bytes needed to store those labels:

  • loki_distributor_bytes_received_total
  • distributor_bytes_received

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

looks awesome )

Base automatically changed from salvacorts/metadata-push-payload to main July 13, 2023 07:13
@salvacorts salvacorts force-pushed the salvacorts/include-metadata-in-bytes-received-metrics branch from 4176298 to a83baaa Compare July 13, 2023 07:25
@salvacorts salvacorts marked this pull request as ready for review July 13, 2023 07:26
@salvacorts salvacorts requested a review from a team as a code owner July 13, 2023 07:26
Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

could you please also check if we have the tests and add the test-case that covers the new logic .
also, could you please update debug log that we have at the bottom of the method to print the size of non-indexed metadata separately.... it might be super useful during the investigation

@salvacorts
Copy link
Contributor Author

could you please update debug log that we have at the bottom of the method to print the size of non-indexed metadata separately.... it might be super useful during the investigation

@vlad-diachenko Done

could you please also check if we have the tests and add the test-case that covers the new logic .

As far as I can see, we are not testing the bytesIngested and bytesReceivedStats metrics anyway. Unfortunately, I don't see a way to test out those metrics. Happy to learn about how to do so! Do we have any examples? I couldn't find any.

@vlad-diachenko
Copy link
Contributor

@salvacorts

As far as I can see, we are not testing the bytesIngested and bytesReceivedStats metrics anyway. Unfortunately, I don't see a way to test out those metrics. Happy to learn about how to do so! Do we have any examples? I couldn't find any.

In general we use testutil.GatherAndCompare() to check the metrics, so, you can use unique tenant id in this test and check the metric from prometheus.DefaultRegistry to check the size is calculated correctly.

@sandeepsukhani
Copy link
Contributor

As far as I can see, we are not testing the bytesIngested and bytesReceivedStats metrics anyway. Unfortunately, I don't see a way to test out those metrics. Happy to learn about how to do so! Do we have any examples? I couldn't find any.

You can also use testutil from Prometheus to get the current value from registered metrics. You can find a couple of examples in the tests here.

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.

The changes look good to me, left just a minor nit. Let us add some tests as suggested by Vlad.

pkg/loghttp/push/push.go Outdated Show resolved Hide resolved
salvacorts and others added 2 commits July 14, 2023 12:21
@pull-request-size pull-request-size bot added size/L and removed size/S labels Jul 14, 2023
@salvacorts
Copy link
Contributor Author

Thank you @sandeepsukhani , I've added a test for the metrics :)

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

@salvacorts salvacorts merged commit 88c17ed into main Jul 17, 2023
@salvacorts salvacorts deleted the salvacorts/include-metadata-in-bytes-received-metrics branch July 17, 2023 07:52
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