Repository navigation
Ignore foreign .db files in the multiprocess directory - #1238
Open
Om-singhaI wants to merge 1 commit into
Open
Om-singhaI wants to merge 1 commit into
Om-singhaI wants to merge 1 commit into
Conversation
MultiProcessCollector.collect() read every *.db file in PROMETHEUS_MULTIPROC_DIR and parsed it as a metric file. Another database in the same directory, such as a SQLite file, made collection fail with struct.error. Because the first 4 bytes of a file are taken as its used size, a large foreign file could also be read into memory in full. Only collect files named the way MultiProcessValue writes them: counter, histogram or summary, or gauge plus a known multiprocess mode, followed by the process identifier. The identifier can be any string, so custom process_identifier functions keep working. Signed-off-by: Om Singhal <wengsinghal@gmail.com>
This branch has not been deployed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #878
MultiProcessCollector.collect()globbed every*.dbfile inPROMETHEUS_MULTIPROC_DIRand parsed each one as a metric file, so another database in the same directory breaks collection. I reproduced it on master with a small SQLite file next to a counter file:collect()raisedstruct.error: unpack_from requires a buffer of at least 1634562704 bytes. The reader takes the first 4 bytes of a file as its used size, which for a SQLite header is about 1.8 GB, so a large foreign file could also be read into memory up to that size.With this change
collect()only reads files named the wayMultiProcessValuewrites them:counter,histogramorsummary, orgaugeplus one of the modes inGauge._MULTIPROC_MODES, then the process identifier and.db. The identifier can be any string, so a customprocess_identifierstill works. Other files are skipped.merge()is unchanged, so callers that pass their own file list see the same behavior as before.mark_process_dead()already removes only exactgauge_<mode>_<pid>.dbnames, so it needed no change.The new test writes a counter and a
livesumgauge under two process identifiers (one of them a string), then adds four junk files:app.db,cache_0.db,counter.dbandgauge_unknown_0.db. It fails on master with thestruct.errorabove and passes with the fix.pytest tests/test_multiprocess.py tests/test_core.pypasses, and flake8 and isort are clean on the changed files.A dedicated directory is still the right setup, as you said in the issue. This just keeps a stray file from taking the metrics endpoint down.
@csmarchbanks could you take a look when you have time?