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 fetched chunk from store size in metric #8971

Merged

Conversation

dannykopping
Copy link
Contributor

@dannykopping dannykopping commented Mar 31, 2023

What this PR does / why we need it:
The Size() function on chunk.Chunk calls out to func (ChunkRef) Size() int in the proto, which doesn't actually include the size of the chunk data! We need to call chunk.Data.Size() for that.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
This will fix the Cache.Chunk.BytesSent stat which indicates how many chunk bytes we sent to the chunk cache. This should probably be renamed for clarity at some point; it's a little confusing.

The same fix is applies to the loki_chunk_fetcher_fetched_size_bytes{source="store"} histogram metric.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
@dannykopping dannykopping force-pushed the dannykopping/fix-fetched-chunk-size branch from dce4cc7 to b0c07ab Compare March 31, 2023 08:18
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
@dannykopping dannykopping force-pushed the dannykopping/fix-fetched-chunk-size branch from b0c07ab to 70e578b Compare March 31, 2023 08:19
@dannykopping dannykopping marked this pull request as ready for review March 31, 2023 08:20
@dannykopping dannykopping requested a review from a team as a code owner March 31, 2023 08:20
Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

LGTM. Nice catch!

@dannykopping dannykopping merged commit de84737 into grafana:main Mar 31, 2023
@dannykopping dannykopping deleted the dannykopping/fix-fetched-chunk-size branch March 31, 2023 08:36
@dannykopping dannykopping added kind/bug backport release-2.7.x add to a PR to backport it into release 2.7.x backport release-2.8.x labels Mar 31, 2023
@grafanabot
Copy link
Collaborator

Hello @dannykopping!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

1 similar comment
@grafanabot
Copy link
Collaborator

Hello @dannykopping!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@dannykopping dannykopping added type/bug Somehing is not working as expected and removed kind/bug labels Mar 31, 2023
@dannykopping dannykopping added product-approved backport release-2.7.x add to a PR to backport it into release 2.7.x and removed backport release-2.8.x backport release-2.7.x add to a PR to backport it into release 2.7.x labels Mar 31, 2023
@grafanabot
Copy link
Collaborator

The backport to release-2.7.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-8971-to-release-2.7.x origin/release-2.7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x de84737368914fa240108c0a3ef843eb5b5e4d3b
# Push it to GitHub
git push --set-upstream origin backport-8971-to-release-2.7.x
git switch main
# Remove the local backport branch
git branch -D backport-8971-to-release-2.7.x

Then, create a pull request where the base branch is release-2.7.x and the compare/head branch is backport-8971-to-release-2.7.x.

@dannykopping dannykopping added backport release-2.8.x and removed backport release-2.7.x add to a PR to backport it into release 2.7.x labels Mar 31, 2023
grafanabot pushed a commit that referenced this pull request Mar 31, 2023
**What this PR does / why we need it**:
The `Size()` function on `chunk.Chunk` calls out to `func (ChunkRef)
Size() int` in the proto, which doesn't actually include the size of the
chunk data! We need to call `chunk.Data.Size()` for that.

(cherry picked from commit de84737)
dannykopping pushed a commit that referenced this pull request Mar 31, 2023
Backport de84737 from #8971

Co-authored-by: Danny Kopping <danny.kopping@grafana.com>
dannykopping pushed a commit that referenced this pull request Apr 3, 2023
**What this PR does / why we need it**:
The `Size()` function on `chunk.Chunk` calls out to `func (ChunkRef)
Size() int` in the proto, which doesn't actually include the size of the
chunk data! We need to call `chunk.Data.Size()` for that.

(cherry picked from commit de84737)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants