Validate diskFrontier domain for series candidate. #115
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
ghost
assigned 
|
Can't really say what those changes are exactly doing, but what I understand looks reasonable. So 👍 |
|
@matttproud I found out the problems with the rules tests: there are two pernicious bugs in fingerprinting code. I'll send PRs to fix them. |
|
Oof. Can't wait to see the P.R.
|
|
Please rebase on master once #121 is merged. |
It is the case with the benchmark tool that we thought that we generated multiple series and saved them to the disk as such, when in reality, we overwrote the fields of the outgoing metrics via Go map reference behavior. This was accidental. In the course of diagnosing this, a few errors were found: 1. ``newSeriesFrontier`` should check to see if the candidate fingerprint is within the given domain of the ``diskFrontier``. If not, as the contract in the docstring stipulates, a ``nil`` ``seriesFrontier`` should be emitted. 2. In the interests of aiding debugging, the raw LevelDB ``levigoIterator`` type now includes a helpful forensics ``String()`` method. This work produced additional cleanups: 1. ``Close() error`` with the storage stack is technically incorrect, since nowhere in the bowels of it does an error actually occur. The interface has been simplified to remove this for now.
| @@ -298,8 +295,6 @@ func (f *memoryToDiskFlusher) ForStream(stream stream) (decoder storage.RecordDe | |||
| flusher: f, | |||
| } | |||
|
|
|||
| // fmt.Printf("fingerprint -> %s\n", model.NewFingerprintFromMetric(stream.metric).ToRowKey()) | |||
There was a problem hiding this comment.
Oh duh, it's removed, not added. Ignore.
|
👍 |
matttproud
added a commit
that referenced
this pull request
Apr 9, 2013
Validate diskFrontier domain for series candidate.
simonpasquier
pushed a commit
to simonpasquier/prometheus
that referenced
this pull request
Oct 12, 2017
List histogram in the metric types overview
codesome
pushed a commit
to codesome/prometheus
that referenced
this pull request
Jul 29, 2019
Fix compactions
krya-kryak
pushed a commit
to krya-kryak/prometheus
that referenced
this pull request
Mar 4, 2022
Export `HTTPClientConfig.validate()`
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.
Code Review
@juliusv please note that b87766021e70#L0R98 causes ./rules/... tests to abort. Could you please have a look at that?
Change Description
It is the case with the benchmark tool that we thought that we
generated multiple series and saved them to the disk as such, when
in reality, we overwrote the fields of the outgoing metrics via
Go map reference behavior. This was accidental. In the course of
diagnosing this, a few errors were found:
newSeriesFrontiershould check to see if the candidate fingerprint is within the given domain of thediskFrontier. If not, as the contract in the docstring stipulates, anilseriesFrontiershould be emitted.levigoIteratortype now includes a helpful forensicsString()method.This work produced additional cleanups:
Close() errorwith the storage stack is technically incorrect, since nowhere in the bowels of it does an error actually occur. The interface has been simplified to remove this for now.