Promtail WAL support: Implement writer side #8267

thepalbi · 2023-01-24T22:40:14Z

What this PR does / why we need it:

This PR is the first in a series that will implement WAL support into Promtail. The main objective of this PR is implement the writer side of WAL.
Note that enabling WAL with the implementation as is will cause entries scraped to be written to WAL, but not remote written. Therefore, should not be used until the whole implementation is completed.

Apart form this, this PR includes a new primitive that will be extender in the following editions: client.Manager. This new component will be responsible for instantiating the correct remote write client, and handle the orchestration from the channel scraping targets write to, to the clients themselves. Also, will handle the graceful shutdown of bespoke clients.

Which issue(s) this PR fixes:
Part of #8197

Special notes for your reviewer:

Since there's a licensing conflict between primitives under the ingester package (AGPL), dependencies have been copied over under promtail, which is Apache.

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

grafanabot · 2023-01-24T22:56:44Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

clients/pkg/promtail/wal/reader.go

clients/pkg/promtail/client/manager.go

pkg/storage/stores/series_store_write_test.go

clients/pkg/promtail/wal/reader.go

clients/pkg/promtail/client/manager.go

clients/pkg/promtail/client/client_test.go

clients/pkg/promtail/wal/encoding.go

grafanabot · 2023-01-26T18:04:34Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-27T17:59:21Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

cstyan · 2023-01-28T00:18:34Z

clients/pkg/promtail/promtail.go

+	var entriesSink api.EntryHandler = p.client
+
+	// If WAL is enabled, instantiate the WAL itself, it's writer, and use that as entries sink for all scraping targets
+	if cfg.WAL.Enabled {
+		pWAL, err := wal.New(cfg.WAL, p.logger, p.reg)
+		if err != nil {
+			return fmt.Errorf("error starting WAL: %w", err)
+		}
+		p.walWriter = wal.NewWriter(pWAL, p.logger, p.client)
+		entriesSink = p.walWriter
+	}
+
+	tms, err := targets.NewTargetManagers(p, p.reg, p.logger, cfg.PositionsConfig, entriesSink, cfg.ScrapeConfig, &cfg.TargetConfig)


I think it probably makes more sense to have promtail have a field that is an entry handler, and during the loading of the config here we create an entry handler that wraps both the list of clients and the walWriter, rather than doing the propagation of the channel data within the wal writer.

Yhea same, wasn't that happy with how it looked adding the fanout inside the writer. Moved it to promtail.go, and added a new slim component that does the fanout.

In this commit the fanout is moved out of the Writer, and done in promtail.go via a EntryHandlerFanouter, component that takes n entry handlers and exposes a single one, performing a fanout. Could be re-used when the client.Manager needs to implement the same functionality as the current multiclient.

clients/pkg/promtail/client/manager.go

grafanabot · 2023-01-30T17:30:54Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

thepalbi · 2023-01-31T13:17:07Z

@cstyan , the latest changes should be from this commit onwards. I tried to remove all unnecessary changes after the rebase, and sorted all imports to make the reviewal process easier. To give a summary for what I changes since last time:

Do fanout to wal writer and client manager as mentioned here
Implemented simple old wal segments cleaner, this is the starting commit, but you could just look for in under pkg/promtail/wal/writer.go
Cleanups

I implemented the old segments cleanup in this PR to at least support alongside with this PR, a very simple cleanup system allows a wal-enabled promtail to run without running the host out of disk

clients/pkg/promtail/wal/config.go

clients/pkg/promtail/wal/writer.go

clients/pkg/promtail/utils/entries.go

clients/pkg/promtail/wal/writer.go

cstyan

almost there, I think we just need a nice way to shutdown the fanout handler and ensure data that was queued in it's channel is sent

clients/pkg/promtail/utils/entries.go

cstyan · 2023-02-03T00:37:44Z

clients/pkg/promtail/promtail.go

+	// these are the sinks were all scraped log entries should get to
+	var entryHandlers = []api.EntryHandler{p.client}
+
+	// If WAL is enabled, instantiate the WAL itself, it's writer, and use that as entries sink for all scraping targets
+	if cfg.WAL.Enabled {
+		p.walWriter, err = wal.NewWriter(cfg.WAL, p.logger, p.reg)
+		if err != nil {
+			return fmt.Errorf("failed to create wal writer: %w", err)
+		}
+		entryHandlers = append(entryHandlers, p.walWriter)
+	}
+
+	p.entriesFanout = utils.NewFanoutEntryHandler(timeoutUntilFanoutHardStop, entryHandlers...)
+
+	tms, err := targets.NewTargetManagers(p, p.reg, p.logger, cfg.PositionsConfig, p.entriesFanout, cfg.ScrapeConfig, &cfg.TargetConfig)


last comment, I think; it feels to me like the creation of the WAL writer should be above within the previous else if cfg.WAL.Enabled, and we could reassign the fanout writer to p.client

if the usage of the actual Client type for p.client makes that difficult lets just find a way to combine these two if statements so that we don't make our job harder when refactoring, we could make p.walWriter a pointer, and check if the pointer isn't null when deciding whether to create the entries fanout?

Yhea also wasn't fond of that double if now that you mention. Moved a bit the code so that we have a single if branch related to wal-enabled behaviour. I think we can keep the fanout if wal is disabled, since it adds the graceful shutdown which is not there otherwise.

Also, we should keep a Client implementing interface in p.client, since it's part of the interface the main promtail objects exposes. In the future, this will be just the client.Manager.

Let me know wdyt!

cstyan

LGTM, thanks for all your hard work here @thepalbi

I'll wait until Monday morning before merging just in case any else has been meaning to take a look at this.

thepalbi requested a review from a team as a code owner January 24, 2023 22:40

pull-request-size bot added the size/XL label Jan 24, 2023

thepalbi requested a review from cstyan January 24, 2023 22:40

thepalbi mentioned this pull request Jan 24, 2023

WAL support for Promtail #8197

Closed

11 tasks

thepalbi force-pushed the pablo/wal-part-1 branch from 75ef8cb to ce33a77 Compare January 24, 2023 22:52

cstyan reviewed Jan 25, 2023

View reviewed changes

pull-request-size bot added size/XXL and removed size/XL labels Jan 26, 2023

thepalbi commented Jan 26, 2023

View reviewed changes

clients/pkg/promtail/client/client_test.go Show resolved Hide resolved

clients/pkg/promtail/wal/encoding.go Outdated Show resolved Hide resolved

thepalbi force-pushed the pablo/wal-part-1 branch from 7d1d5bd to 06582e6 Compare January 26, 2023 18:00

cstyan reviewed Jan 28, 2023

View reviewed changes

kgeckhart reviewed Jan 30, 2023

View reviewed changes

clients/pkg/promtail/client/manager.go Outdated Show resolved Hide resolved

thepalbi force-pushed the pablo/wal-part-1 branch from f71df99 to 12fd129 Compare January 30, 2023 17:23

thepalbi added 12 commits January 30, 2023 19:49

relicensed WAL main definitions

a9bd4d5

relicensed ingester wal code

33472a7

remove unnecessary commit noise

f7af55d

imports order

2dae764

one more fix

5f32685

part 1 implemented

bcd862d

WIP

32675e9

lint passing

c9a5c47

remove unnecessary changes

1e1b1cb

post rebase changes

c2b89ea

fix licensing issues

5827391

PR fixes wip. Tests not working

e2db382

thepalbi added 9 commits January 31, 2023 09:54

implemented simple old segments cleaner

4420525

post-rebase fixes

1bdfa40

moved clients fanout out of writer

5af6e20

use config name in repeated client error

9b9cc79

graceful shutdown of writer

0ee9c6c

tested writer clean up

e56249a

clear up naming

ac99782

added reclaimed space metric

e2bdad0

post rebase fixes

590c0f7

thepalbi force-pushed the pablo/wal-part-1 branch from 929f903 to 590c0f7 Compare January 31, 2023 13:07

fix linter

ba679c5

cstyan reviewed Jan 31, 2023

View reviewed changes

thepalbi and others added 4 commits January 31, 2023 18:15

removing unused stuff

b94a3cf

concurrent fanout entry handler

7773395

likely deprecation notice

4d986f3

Merge branch 'main' into pablo/wal-part-1

5128fc4

cstyan reviewed Feb 1, 2023

View reviewed changes

clients/pkg/promtail/utils/entries.go Outdated Show resolved Hide resolved

clients/pkg/promtail/utils/entries.go Outdated Show resolved Hide resolved

thepalbi and others added 4 commits February 1, 2023 15:01

fanout with timeout

63a73da

cleaning up hard stop on fanouter

3a420d3

naming on const

29903c5

Merge branch 'main' into pablo/wal-part-1

42171ed

cstyan reviewed Feb 3, 2023

View reviewed changes

thepalbi and others added 2 commits February 3, 2023 17:47

refactor promtail.go wal writer into one if branch

b8bb6d4

Merge branch 'main' into pablo/wal-part-1

1ac07a0

cstyan approved these changes Feb 3, 2023

View reviewed changes

thepalbi and others added 2 commits February 6, 2023 16:23

Merge branch 'main' into pablo/wal-part-1

cb5dc2b

Merge branch 'main' into pablo/wal-part-1

bb1d2c0

cstyan merged commit b5ed39c into main Feb 6, 2023

cstyan deleted the pablo/wal-part-1 branch February 6, 2023 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promtail WAL support: Implement writer side #8267

Promtail WAL support: Implement writer side #8267

thepalbi commented Jan 24, 2023 •

edited

Loading

grafanabot commented Jan 24, 2023

grafanabot commented Jan 26, 2023

grafanabot commented Jan 27, 2023

cstyan Jan 28, 2023

thepalbi Jan 30, 2023

thepalbi Jan 31, 2023

grafanabot commented Jan 30, 2023

thepalbi commented Jan 31, 2023

cstyan left a comment

cstyan Feb 3, 2023

thepalbi Feb 3, 2023

cstyan left a comment

Promtail WAL support: Implement writer side #8267

Promtail WAL support: Implement writer side #8267

Conversation

thepalbi commented Jan 24, 2023 • edited Loading

grafanabot commented Jan 24, 2023

grafanabot commented Jan 26, 2023

grafanabot commented Jan 27, 2023

cstyan Jan 28, 2023

Choose a reason for hiding this comment

thepalbi Jan 30, 2023

Choose a reason for hiding this comment

thepalbi Jan 31, 2023

Choose a reason for hiding this comment

grafanabot commented Jan 30, 2023

thepalbi commented Jan 31, 2023

cstyan left a comment

Choose a reason for hiding this comment

cstyan Feb 3, 2023

Choose a reason for hiding this comment

thepalbi Feb 3, 2023

Choose a reason for hiding this comment

cstyan left a comment

Choose a reason for hiding this comment

thepalbi commented Jan 24, 2023 •

edited

Loading