Bump loki.write code to latest client manager changes
#4483
Conversation
thepalbi
changed the title
loki.write code to latest client manager changes
| ReadlineRate float64 `mapstructure:"readline_rate" yaml:"readline_rate" json:"readline_rate"` | ||
| ReadlineBurst int `mapstructure:"readline_burst" yaml:"readline_burst" json:"readline_burst"` | ||
| ReadlineRateEnabled bool `mapstructure:"readline_rate_enabled,omitempty" yaml:"readline_rate_enabled,omitempty" json:"readline_rate_enabled"` | ||
| ReadlineRateDrop bool `mapstructure:"readline_rate_drop,omitempty" yaml:"readline_rate_drop,omitempty" json:"readline_rate_drop"` | ||
| MaxStreams int `mapstructure:"max_streams" yaml:"max_streams" json:"max_streams"` | ||
| MaxLineSize flagext.ByteSize `mapstructure:"max_line_size" yaml:"max_line_size" json:"max_line_size"` | ||
| MaxLineSizeTruncate bool `mapstructure:"max_line_size_truncate" yaml:"max_line_size_truncate" json:"max_line_size_truncate"` |
There was a problem hiding this comment.
nit: I know we probably copied these from loki, but could we remove the field tags here? Maybe also use units.Base2Bytes instead of flagext.ByteSize?
edit: maybe we can postpone this for later on, as we're tidying up all test files and structs, no need to do something right now.
There was a problem hiding this comment.
I can leave it for a follow up is that's ok. Creates this to track a couple
| m.streamLag = prometheus.NewGaugeVec(prometheus.GaugeOpts{ | ||
| Name: "loki_write_stream_lag_seconds", | ||
| Help: "Difference between current time and last batch timestamp for successful sends", |
There was a problem hiding this comment.
Why did we end up removing the streamLag metric? Is it because the WAL implementation will not be synchronous and thus will have no meaning?
There was a problem hiding this comment.
So the removal of that metrics was included in the forklifted changes from upstream (loki). See this commit
@tpaschalis took me about a day of wrangling with the code (fixing types, some tests, race's that appeared here and not in loki (mainly becuase it's enabled). After discussing with @cstyan , I'll change the approach for further work improving loki's remote write: The idea is to first do work in this repo, and then backport the changes (could be in batch) to Promtail. Since this is the primary logs agent we consume, it makes sense to make develop here first, since it will be more battle-tested. I think that will solve mostly all pains since this is the place we needs changes ASAP, and we can figure out how to better manage the backport later. |
github-actions
bot
added
the
frozen-due-to-age
PR Description
This PR bumps the Promtail code used in the
loki.writecomponent to the latest changes (this commit that refactors all remote write client types into once client manager). This allows us to use the newclient.Managerfor orchestrating multiple remote-write clients. By doing this, for example, we can replace the inlined fanfout client implementation inside the component with the one implemented inside the client.Manager.A follow up PR will expose river support for enabling the WAL within the manager, which is configured through this struct.
At the moment, this PR brings no new functionality, since the remote write clients are configured the same.
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist