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

Implement WAL replay and markers for loki.write #5590

Merged
merged 31 commits into from
Nov 2, 2023

Conversation

thepalbi
Copy link
Contributor

@thepalbi thepalbi commented Oct 24, 2023

PR Description

This PR branches of #5434, and applies the work done in ptodev/prometheus#1 to improve two things:

  1. WAL replay is implemented for loki.write, so upon an agent restart, it will replay "not sent" data saved in the WAL segments that are alive
  2. Implement a "markers" mechanism that keeps track of which was the last consumed segment, consumed being read and delivered to the RW destination. This mechanisms works by creating a feedback loop between the side that reads from the WAL, and the side that sends batches.

TODOs

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@thepalbi thepalbi requested a review from a team as a code owner October 24, 2023 19:33
@thepalbi thepalbi marked this pull request as draft October 24, 2023 19:33
@thepalbi thepalbi mentioned this pull request Oct 24, 2023
5 tasks
@thepalbi thepalbi changed the title Pablo/wal replay and markers from paulin Implement WAL replay and markers for loki.write1 Oct 24, 2023
@thepalbi thepalbi changed the title Implement WAL replay and markers for loki.write1 Implement WAL replay and markers for loki.write Oct 24, 2023
Copy link
Contributor

@ptodev ptodev left a comment

Choose a reason for hiding this comment

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

Thank you for this! I am super happy for this change. IMO it doesn't have to be super polished in order to be merged, but I just have a few main concerns:

  • I'd like us to protect ourselves from corrupted marker files.
  • Some of the new interfaces could use a few more comments :)
  • I guess performance isn't impacted much, and we don't have to run any special benchmarks?

component/common/loki/client/batch.go Outdated Show resolved Hide resolved
component/common/loki/client/internal/marker_handler.go Outdated Show resolved Hide resolved
component/common/loki/wal/watcher.go Outdated Show resolved Hide resolved
component/common/loki/wal/watcher.go Show resolved Hide resolved
component/common/loki/wal/watcher.go Outdated Show resolved Hide resolved
component/loki/write/types.go Outdated Show resolved Hide resolved
component/common/loki/client/queue_client.go Outdated Show resolved Hide resolved
@thepalbi thepalbi force-pushed the pablo/wal-replay-and-markers branch from 2f59c24 to 26d4e6b Compare October 26, 2023 19:48
@thepalbi
Copy link
Contributor Author

thepalbi commented Oct 26, 2023

@ptodev went through ~half the comments, and rebased over the latest changes in #5434, which is already approved. Will finish first thing tomorrow, left some throughts on the marker's format versioning comments.

Base automatically changed from pablo/wal-replay to main October 27, 2023 13:06
@thepalbi thepalbi force-pushed the pablo/wal-replay-and-markers branch from 26d4e6b to f3cde09 Compare October 27, 2023 18:03
@thepalbi thepalbi marked this pull request as ready for review October 30, 2023 16:42
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone deletes the WAL, but does not delete the marker file, would the WAL replay code work ok? I suppose in that case the code should know to ignore the segment ID in the marker file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the marking logic is not connected to the WAL, so it can tell if it was deleted or not. I guess for WAL deletions we should advice that the marker has to be deleted as well.

On the other hand, the watcher, once it reads from the marker, tries to find the following segment by looking into disk, so since the WAL doesn't exists it will enter a retry loop until the WAL get's re-created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could add an additional logic to the marker interface, so the WAL can force write to the marker file. Like if it detects WAL deletion, force mark a segment

component/common/loki/client/internal/marker_handler.go Outdated Show resolved Hide resolved
thepalbi and others added 8 commits November 1, 2023 09:53
* encoding wip

* using in LastMarkedSegment

* atomic marker write

* Apply suggestions from code review

Co-authored-by: Paulin Todev <[email protected]>

---------

Co-authored-by: Paulin Todev <[email protected]>
@thepalbi
Copy link
Contributor Author

thepalbi commented Nov 1, 2023

@ptodev @mattdurham just added on top of the previous queue client benchmarks, once comparing the nil marker and real marker implementations. No significant diff whatsoever:

goos: darwin
goarch: arm64
pkg: github.com/grafana/agent/component/common/loki/client
                                                                    │ queue_nil_marker_handler │       queue_marker_handler       │
                                                                    │          sec/op          │   sec/op    vs base              │
ClientImplementations/100_entries,_single_series,_no_batching-10                    1.001 ± 0%   1.001 ± 0%       ~ (p=0.394 n=6)
ClientImplementations/100k_entries,_100_series,_default_batching-10                 4.292 ± 1%   4.336 ± 1%  +1.02% (p=0.026 n=6)
geomean                                                                             2.073        2.084       +0.50%

                                                                    │ queue_nil_marker_handler │        queue_marker_handler        │
                                                                    │           B/op           │     B/op      vs base              │
ClientImplementations/100_entries,_single_series,_no_batching-10                  861.0Ki ± 5%   861.6Ki ± 5%       ~ (p=0.615 n=6)
ClientImplementations/100k_entries,_100_series,_default_batching-10               788.9Mi ± 0%   788.0Mi ± 0%  -0.12% (p=0.002 n=6)
geomean                                                                           25.75Mi        25.75Mi       -0.02%

                                                                    │ queue_nil_marker_handler │       queue_marker_handler        │
                                                                    │        allocs/op         │  allocs/op   vs base              │
ClientImplementations/100_entries,_single_series,_no_batching-10                   11.26k ± 1%   11.28k ± 0%       ~ (p=0.058 n=6)
ClientImplementations/100k_entries,_100_series,_default_batching-10                10.80M ± 0%   10.80M ± 0%  -0.01% (p=0.002 n=6)
geomean                                                                            348.8k        349.1k       +0.09%

@@ -58,6 +59,15 @@ func NewWatcherMetrics(reg prometheus.Registerer) *WatcherMetrics {
},
[]string{"id"},
),
replaySegment: prometheus.NewGaugeVec(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have some additional metrics that would be handy but if you want to do them in another more focused PR thats fine. WAL size in bytes, and WAL oldest timestamp and current timestamp. Kind of like segment but its more actionable for alerts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Down for checking that in another PR, but since our WAL writer uses prometheus'es wlog implementation, we inherit all this metrics https://github.com/prometheus/prometheus/blob/3450572840881c870012ef80bff1877568698638/tsdb/wlog/wlog.go#L196-L205. I think we don't have timestamps, so that one might be interesting for alerting on WAL not being written or too far behind (like to measure lag from wal write to watcher read).

@thepalbi
Copy link
Contributor Author

thepalbi commented Nov 2, 2023

@mattdurham @ptodev build is green, and all convos are resolved, can you take a final pass when you have some time?

@mattdurham mattdurham self-requested a review November 2, 2023 14:24
Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

There are some small nits and a few side conversations but looking solid.

@mattdurham mattdurham merged commit abdaca0 into main Nov 2, 2023
@mattdurham mattdurham deleted the pablo/wal-replay-and-markers branch November 2, 2023 14:29
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants