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

Backfill from OpenMetrics format #8084

Merged
merged 98 commits into from
Nov 26, 2020
Merged

Backfill from OpenMetrics format #8084

merged 98 commits into from
Nov 26, 2020

Conversation

asquare14
Copy link
Contributor

Related PR : #7586
Design Document here

@roidelapluie roidelapluie added this to In progress in backfilling Oct 20, 2020
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
@bwplotka bwplotka self-requested a review October 29, 2020 13:40
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Some initial comments.

Good job, thanks!

.gitignore Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import_test.go Outdated Show resolved Hide resolved
tsdb/importer/openmetrics/openmetrics.go Outdated Show resolved Hide resolved
tsdb/importer/openmetrics/openmetrics.go Outdated Show resolved Hide resolved
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Just saw that @bwplotka has submitted feedback. I was in the middle of it, but then the dev summit started. I'll just submit my comments so far without further adjusting. (They were mostly nitpicking anyway.) Will then do the rest of my review on top of @bwplotka .

@asquare14 assume that I agree with anything @bwplotka said unless I explicitly say something else. (o:

.gitignore Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
test.om Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

OK, a few more points. Looks like the generally correct approach.

cmd/promtool/tsdb.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
cmd/promtool/main.go Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/openmetrics/openmetrics.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
cmd/promtool/tsdb.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/import.go Outdated Show resolved Hide resolved
tsdb/importer/openmetrics/openmetrics.go Outdated Show resolved Hide resolved
@asquare14 asquare14 marked this pull request as draft October 30, 2020 10:38
Move backfill from tsdb to promtool directory

Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Rename ContenType

Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
@beorn7
Copy link
Member

beorn7 commented Nov 2, 2020

Expanding on ideas about the command structure we just brainstormed during the sync meeting:

As discussed, the command should not be import (and neither backfill, as in #7675) but something that is both more appropriate and applies to both. Our best idea so far is create-blocks-from, with the next sub-command telling what (e.g. openmetrics, rules). I think we should also try to stay as faithful as possible to existing flags and args conventions in the promtool commands. For example, promtool tsdb (dump|list|analyze) all use an argument (called "db path") to define where the TSDB files are. Here and in #7675, we use a flag.

Backfilling from rules: promtool tsdb create-blocks-from rules [<flags>] [<db path>]
Backfilling from OM file: promtool tsdb create-blocks-from openmetrics [<flags>] [<OM file>] [<db path>]

As for the other tsdb commands, the default value for <db path> is defaultDBPath, i.e. data/`.

The flag common to both sub-commands is block-length (hidden, default 2h).

The rules sub-command has the additional flags url (server to evaluate against), start, end, step, rule-file (ExistingFiles(), i.e. repeatable).

Since we want to allow multiple rule files for rule backfilling, but only one input file for backfilling from a file, we use a flag for the former but an argument for the latter.

Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Small nits, overall Looks good! 💪

cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill.go Show resolved Hide resolved
cmd/promtool/backfill.go Show resolved Hide resolved
cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill_test.go Outdated Show resolved Hide resolved
tsdb/blockwriter.go Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Just some comment nits, otherwise this LGTM.

But please, other reviewers, could you quickly doublecheck if your concerns are all addressed?

cmd/promtool/backfill.go Show resolved Hide resolved
tsdb/blockwriter.go Outdated Show resolved Hide resolved
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

Lots of nits with 1 small fix in the test :)

cmd/promtool/backfill.go Show resolved Hide resolved
cmd/promtool/backfill.go Show resolved Hide resolved
cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill_test.go Outdated Show resolved Hide resolved
cmd/promtool/backfill_test.go Outdated Show resolved Hide resolved
cmd/promtool/backfill_test.go Outdated Show resolved Hide resolved
cmd/promtool/backfill_test.go Outdated Show resolved Hide resolved
cmd/promtool/backfill_test.go Outdated Show resolved Hide resolved
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

I was about to hit merge but found 2 bugs in the last sanity check :)

cmd/promtool/backfill.go Outdated Show resolved Hide resolved
cmd/promtool/backfill_test.go Show resolved Hide resolved
cmd/promtool/tsdb.go Outdated Show resolved Hide resolved
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

LGTM!

@codesome codesome merged commit b317b6a into prometheus:master Nov 26, 2020
@codesome codesome moved this from In progress to Done in backfilling Nov 26, 2020
@asquare14 asquare14 mentioned this pull request Nov 27, 2020
@pathcl
Copy link

pathcl commented Sep 2, 2021

@bwplotka hey there! Im trying to use what you described here: https://grafana.com/blog/2020/09/02/how-were-improving-backfill-methods-to-get-older-data-into-prometheus/ however I wonder what would be the way to import csv nowadays given all these pull request merged. I did noticed this: https://github.com/prometheus/prometheus/blob/main/docs/storage.md but can't find an explicit example of importing a csv file as it was described in your talk/demo session.

Thanks a lot !!

@beorn7
Copy link
Member

beorn7 commented Sep 2, 2021

There is no direct way of importing metrics data from CSV yet. As it turned out, CSV is really not very well suited for metrics data, which makes it hard to come up with a good generally useful approach. Having said that, it's still on the agenda, we just didn't do it as the first thing.

The linked design doc has a bunch of concerns about CSV in the discussion. Contributions are welcome, but best to be discussed via the community channels first. (Questions and discussions on a closed GH PR easily get lost.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants