Reuse the same appender for report and scrape #7562

roidelapluie · 2020-07-11T22:28:12Z

Fix #6950

Signed-off-by: Julien Pivotto [email protected]

Signed-off-by: Julien Pivotto <[email protected]>

roidelapluie · 2020-07-12T19:42:21Z

/prombench master

prombot · 2020-07-12T19:42:22Z

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-7562 and master

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart master

roidelapluie · 2020-07-13T08:34:57Z

/prombench cancel

prombot · 2020-07-13T08:34:58Z

Benchmark cancel is in progress.

Signed-off-by: Julien Pivotto <[email protected]>

beorn7

AFAICS, this looks fine. I have only a few nits that don't change how the code is working.

@codesome could you also run a sanity check on this?

scrape/helpers_test.go

scrape/scrape.go

Signed-off-by: Julien Pivotto <[email protected]>

roidelapluie · 2020-07-14T15:18:43Z

Thanks for the review, I have addressed the comments.

beorn7

Cool. Now let's get a 👍 from @codesome @krasi-georgiev or @brian-brazil to not merge this without any of the maintainers approving.

Signed-off-by: Julien Pivotto <[email protected]>

roidelapluie · 2020-07-14T19:31:21Z

@beorn7 I added another commit, which makes the end of run staleness consistent.

Also now it takes into consideration that Commit() will do a Rollback() if things go mad.

brian-brazil

I thought you were still working on this, so hadn't reviewed yet.

brian-brazil · 2020-07-15T11:00:00Z

scrape/scrape_test.go

@@ -1928,3 +1970,66 @@ func TestCheckAddError(t *testing.T) {
 	sl.checkAddError(nil, nil, nil, storage.ErrOutOfOrderSample, nil, &appErrs)
 	testutil.Equals(t, 1, appErrs.numOutOfOrder)
 }
+
+func TestScrapeReportIsolation(t *testing.T) {


I'm not seeing how this is testing isolation, as there's nothing to ensure the query runs between the scrape ingestion and the report ingestion.

Queries run continuously. We should always have a multiple of 9 metrics.

That test is failing almost instantly without the code.

Yes, I was assuming it would happen as @roidelapluie has confirmed now.

The test name is misleading, as this doesn't test anything about isolation per-se. What it's testing is that it's all done in one append.

@roidelapluie that name should be easy to fix.
@brian-brazil is that the only remaining concern in this PR?

Yes, the rest I can live with.

Renamed. I originally named it like this because if the underlying appender does not support isolation, the test failed as well.

brian-brazil · 2020-07-15T11:05:16Z

scrape/scrape.go

-			}
-		}
+// scrapeAndReport performs a scrape and then appends the result to the storage
+// together with reporting metrics, all using the same appender.


"the same" is a little misleading as a 2nd appender will be created if there's a scrape error.

But in case of a scrape error, the 1st appender is rolled back, so it will not append anything to the storage, so only one and the same appender will ever append any samples to the storage.

But if you have a better suggestion for the wording, I'm sure @roidelapluie will appreciate it.

// scrapeAndReport performs a scrape and then appends the result to the storage
// together with reporting metrics, by using as few appenders as possible.
// In the happy scenario, a single appender is used.

brian-brazil · 2020-07-15T11:06:38Z

scrape/scrape.go

-			close(sl.stopped)
+	app := sl.appender()
+	var err error
+	defer func() {


There's no early returns, so I don't see a need to use a defer here compared to putting this code at the end of the function - which would be clearer.

We guard ourselves against potential panic()

Signed-off-by: Julien Pivotto <[email protected]>

beorn7 · 2020-07-16T10:14:43Z

Is this good to go @brian-brazil ? Would be great to have this in the release.

beorn7 · 2020-07-16T10:45:45Z

Or in case @brian-brazil is busy, I guess @codesome or @krasi-georgiev are qualified here, too, as it is close to TSDB.

Signed-off-by: Julien Pivotto <[email protected]>

brian-brazil · 2020-07-16T11:47:20Z

👍

beorn7 · 2020-07-16T12:59:40Z

😄 Thanks everyone.

roidelapluie force-pushed the isolation-test branch from 1767ce0 to 5133c73 Compare July 11, 2020 22:30

roidelapluie changed the title ~~Scrape tests: implement isolation in collectResultAppender~~ Reuse the same appender for report and scrape Jul 12, 2020

roidelapluie force-pushed the isolation-test branch from 3aafe11 to 5b10da8 Compare July 12, 2020 09:14

roidelapluie requested review from beorn7 and brian-brazil July 12, 2020 09:20

roidelapluie force-pushed the isolation-test branch 3 times, most recently from 24d71c4 to 6598956 Compare July 12, 2020 19:16

roidelapluie added 3 commits July 12, 2020 21:41

Scrape tests: implement isolation in collectResultAppender

ee7250b

Signed-off-by: Julien Pivotto <[email protected]>

Reuse the same appender for report and scrape

05c29d0

Signed-off-by: Julien Pivotto <[email protected]>

Add test

caa9fbc

Signed-off-by: Julien Pivotto <[email protected]>

roidelapluie force-pushed the isolation-test branch from 6598956 to caa9fbc Compare July 12, 2020 19:41

prombot added the prombench label Jul 12, 2020

roidelapluie added 4 commits July 14, 2020 00:22

Remove un-needed function call

d8bdcf1

Signed-off-by: Julien Pivotto <[email protected]>

Rename function

fbc0871

Signed-off-by: Julien Pivotto <[email protected]>

Move the function around and add a comment

4b70df9

Signed-off-by: Julien Pivotto <[email protected]>

log errors

c8f6e72

Signed-off-by: Julien Pivotto <[email protected]>

beorn7 requested changes Jul 14, 2020

View reviewed changes

scrape/helpers_test.go Outdated Show resolved Hide resolved

scrape/helpers_test.go Outdated Show resolved Hide resolved

scrape/scrape.go Outdated Show resolved Hide resolved

scrape/scrape.go Outdated Show resolved Hide resolved

Apply comments

ec9b83a

Signed-off-by: Julien Pivotto <[email protected]>

beorn7 approved these changes Jul 14, 2020

View reviewed changes

roidelapluie added 3 commits July 14, 2020 21:23

Rework commit/rollback

c6a104d

Signed-off-by: Julien Pivotto <[email protected]>

Fix logic

41e3ac8

Signed-off-by: Julien Pivotto <[email protected]>

No new alloc

844fb36

Signed-off-by: Julien Pivotto <[email protected]>

brian-brazil reviewed Jul 15, 2020

View reviewed changes

reword

4204288

Signed-off-by: Julien Pivotto <[email protected]>

beorn7 mentioned this pull request Jul 16, 2020

Cut 2.20.0-rc.0 #7583

Merged

Rename test

fd8f29f

Signed-off-by: Julien Pivotto <[email protected]>

roidelapluie merged commit 754461b into prometheus:master Jul 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse the same appender for report and scrape #7562

Reuse the same appender for report and scrape #7562

roidelapluie commented Jul 11, 2020 •

edited

Loading

roidelapluie commented Jul 12, 2020

prombot commented Jul 12, 2020

roidelapluie commented Jul 13, 2020

prombot commented Jul 13, 2020

beorn7 left a comment

roidelapluie commented Jul 14, 2020

beorn7 left a comment

roidelapluie commented Jul 14, 2020

brian-brazil left a comment

brian-brazil Jul 15, 2020

roidelapluie Jul 15, 2020

beorn7 Jul 15, 2020

brian-brazil Jul 16, 2020

beorn7 Jul 16, 2020

brian-brazil Jul 16, 2020

roidelapluie Jul 16, 2020

brian-brazil Jul 15, 2020

beorn7 Jul 15, 2020

roidelapluie Jul 15, 2020

brian-brazil Jul 15, 2020

roidelapluie Jul 15, 2020

beorn7 commented Jul 16, 2020

beorn7 commented Jul 16, 2020

brian-brazil commented Jul 16, 2020

beorn7 commented Jul 16, 2020

Reuse the same appender for report and scrape #7562

Reuse the same appender for report and scrape #7562

Conversation

roidelapluie commented Jul 11, 2020 • edited Loading

roidelapluie commented Jul 12, 2020

prombot commented Jul 12, 2020

roidelapluie commented Jul 13, 2020

prombot commented Jul 13, 2020

beorn7 left a comment

Choose a reason for hiding this comment

roidelapluie commented Jul 14, 2020

beorn7 left a comment

Choose a reason for hiding this comment

roidelapluie commented Jul 14, 2020

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Jul 16, 2020

beorn7 commented Jul 16, 2020

brian-brazil commented Jul 16, 2020

beorn7 commented Jul 16, 2020

roidelapluie commented Jul 11, 2020 •

edited

Loading