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.

Sign up for GitHub

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

Jump to bottom

CORE-2024: local storage probe for compacted away bytes vectorized_storage_log_compaction_removed_bytes #17579

Merged
merged 3 commits into from
Apr 10, 2024
Merged

CORE-2024: local storage probe for compacted away bytes vectorized_storage_log_compaction_removed_bytes #17579

merged 3 commits into from
Apr 10, 2024

Conversation

andijcr
Copy link
Contributor

@andijcr andijcr commented Apr 3, 2024 •
edited

ntp metrics for compaction observability:

vectorized_storage_log_compaction_removed_bytes

a counter metric that hooks in self_compact_segments() and sliding_window_compact() to record size_before - size_after at each successful invocation.

Fixes https://github.com/redpanda-data/core-internal/issues/1232

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

Features

  • new metric vectorized_storage_log_compacted_away_bytes for compaction observability in local storage

@andijcr
Copy link
Contributor Author

andijcr commented Apr 3, 2024

/ci-repeat 1

@andijcr andijcr marked this pull request as ready for review April 3, 2024 15:48
@andijcr andijcr changed the title [Draft] probe for cloud storage compacted away bytes cloud/local storage probes for compacted away bytes Apr 3, 2024
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Apr 3, 2024 •
edited

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47313#018ea4e9-812a-490f-a23f-403f3a9012d0

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47313#018ea4e9-812e-4f96-9987-0912550c291a

andrwng
andrwng reviewed Apr 3, 2024
View reviewed changes
Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

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

Would be great to add a test. You mentioned offline you have a ducktape one coming up, but I'd suggest considering writing a fixture test, since it'll give you more control over what when uploads happen

src/v/cloud_storage/partition_manifest.h Outdated Show resolved Hide resolved
src/v/storage/segment_utils.cc Outdated
Comment on lines 716 to 717
pb.segment_compacted();
pb.add_compacted_away_bytes(sz_before - *sz_after);
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to do this for windowed compaction as well

Copy link
Contributor

Choose a reason for hiding this comment

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

it's guaranteed that sz_before >= *sz_after, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Lazin good point. I had a bit of dive, and I don't see a guarantee in the code or a fuzz test for this (I might be wrong; pls correct me). It seems difficult that the implementation would produce a bigger file, but bugs are not always obvious.

I moved the metric to ssize_t, I wonder if I should also add a warn (or even error) message for this condition

src/v/cloud_storage/partition_manifest.h Outdated Show resolved Hide resolved
@andrwng
Copy link
Contributor

andrwng commented Apr 3, 2024

Also I know these are kind of related, but they seem like pretty independent ideas that don't depend to each other. Especially if you plan on adding tests for them and this PR grows, I think it's worth splitting into a couple PRs.

@andijcr andijcr force-pushed the feat/storage_compression_savings branch from 3e6f7a9 to 0b22295 Compare April 4, 2024 12:44
@andijcr andijcr changed the title cloud/local storage probes for compacted away bytes local storage probe for compacted away bytes vectorized_storage_log_compacted_away_bytes Apr 4, 2024
@andijcr
Copy link
Contributor Author

andijcr commented Apr 4, 2024 •
edited

force push: removed commit not relevant to vectorized_storage_log_compacted_away_bytes

separate pr here #17627

@andijcr andijcr requested a review from andrwng April 4, 2024 14:50
andrwng
andrwng reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

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

Overall looks good! Just a naming suggestion.

You mentioned adding a probe test -- is that for this PR?

src/v/storage/probe.cc Outdated Show resolved Hide resolved
@andijcr
Copy link
Contributor Author

andijcr commented Apr 4, 2024

You mentioned adding a probe test -- is that for this PR?

i'd like to say for both PR. I'll commit it separately on both, and later I'm thinking of a followup to move the find_metric helper under test/utils

Lazin
Lazin previously approved these changes Apr 4, 2024
View reviewed changes
src/v/storage/segment_utils.cc Outdated
Comment on lines 716 to 717
pb.segment_compacted();
pb.add_compacted_away_bytes(sz_before - *sz_after);
Copy link
Contributor

Choose a reason for hiding this comment

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

it's guaranteed that sz_before >= *sz_after, correct?

@dotnwat dotnwat changed the title local storage probe for compacted away bytes vectorized_storage_log_compacted_away_bytes CORE-2024: local storage probe for compacted away bytes vectorized_storage_log_compacted_away_bytes Apr 8, 2024
@andijcr
storage/probe: vectorized_storage_log_compacted_away_bytes
3351557 
new ntp metric to keep track of how many bytes are removed by
compaction
@andijcr
storage/probe: connect add_compacted_away_bytes to compaction
ba176bf 
probe is updated in self_compact_segment
and
sliding_window_compact
@andijcr
t/compaction_e2e_test: metric check during compaction
f2a1a54 
check that vectorized_storage_log_compaction_removed_bytes_total has the
expected behaviour in various compaction configurations

it should not increase for "delete", should increase for "compact" or
"compact,delete"
@andijcr andijcr force-pushed the feat/storage_compression_savings branch from 0b22295 to f2a1a54 Compare April 9, 2024 13:09
@andijcr andijcr changed the title CORE-2024: local storage probe for compacted away bytes vectorized_storage_log_compacted_away_bytes CORE-2024: local storage probe for compacted away bytes vectorized_storage_log_compaction_removed_bytes Apr 9, 2024
@andijcr
Copy link
Contributor Author

andijcr commented Apr 9, 2024

review feedbacks and ducktape test to check that the metric increases

@andijcr andijcr requested review from Lazin and andrwng April 9, 2024 16:33
@andijcr
Copy link
Contributor Author

andijcr commented Apr 9, 2024

failure is #14425

@piyushredpanda piyushredpanda merged commit e5ed934 into redpanda-data:dev Apr 10, 2024
17 of 20 checks passed
@andijcr andijcr deleted the feat/storage_compression_savings branch April 10, 2024 14:15
@andijcr andijcr restored the feat/storage_compression_savings branch April 10, 2024 14:18
dotnwat
dotnwat reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

lgtm

@renovate renovate bot mentioned this pull request May 4, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotnwat dotnwat dotnwat left review comments

@andrwng andrwng andrwng approved these changes

@Lazin Lazin Lazin approved these changes

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@andijcr @vbotbuildovich @andrwng @Lazin @dotnwat @piyushredpanda