consumption_metrics: send timeline_written_size_delta #4822
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As far as testing this thing goes... Unsure if a test should be added. I am quite unsure on how to add non-flaky tests in general :) After overhauling the consumption metrics sending completly we could easily test this out. |
1240 tests run: 1185 passed, 0 failed, 55 skipped (full report) |
vadim2404
reviewed
Jul 27, 2023
shanyp
reviewed
Jul 27, 2023
ololobus
reviewed
Jul 27, 2023
ololobus
reviewed
Jul 27, 2023
koivunej
commented
Jul 31, 2023
koivunej
commented
Jul 31, 2023
ololobus
reviewed
Jul 31, 2023
|
Tagged @lubennikovaav fore review as well. Shany is on vacation and I think that I'm only partially capable of doing review here |
no need to keep them separate anymore.
because why not, assuming it will keep growing with similar ... variants, if there can be any, those should also be copy.
this should give us a data point in the first iteration as well.
this is just the written_size_delta_bytes.
restore the name of factory fn to be unrelated of the suffix.
if we would always use the latest absolute time, it would work until last_record_lsn stops to update because compute is down. after the value no longer grows, we will no longer send it, and thus the cached value will no longer advance. however incremental values are not cache deduplicated, so we would had started the time range for the incremental value from the last absolute value. perhaps this could be the cached incremental value as well. or... perhaps we should never put these incremental values into the cache at all.
koivunej
force-pushed
the
delta_consumption_metric
branch
from
8dbd49d to
821c41d
Compare
lubennikovaav
approved these changes
Jul 31, 2023
|
Writing a unit test for this is possible, but now that I finally have it (monkeyd from #4674), unsure if the errors are because of the tenant harness differences or something else. |
koivunej
commented
Jul 31, 2023
|
Still trying to get the CI to pass 👍 |
koivunej
added a commit
that referenced
this pull request
Aug 1, 2023
Two stabs at this, by mocking a http receiver and the globals out (now reverted) and then by separating the timeline dependency and just testing what kind of events certain timelines produce. I think this pattern could work for some of our problems. Follow-up to #4822.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We want to have timeline_written_size_delta which is defined as difference to the previously sent
timeline_written_sizefrom the currenttimeline_written_size.Solution is to send it. On the first round
disk_consistent_lsnis used which is captured duringloadtime. After that an incremental "event" is sent on every collection. Incremental "events" are not part of deduplication.I've added some infrastructure to allow somewhat typesafe
EventType::AbsoluteandEventType::Incrementalfactories per metrics, now that we have our firstEventType::Incrementalusage.