[MM-55225] add simple scraper logic #4
Conversation
server/const.go
Outdated
| sampleMutator func(labels.Labels) labels.Labels //nolint:unused | ||
| sampleMutator func(labels.Labels) labels.Labels //nolint:unused | ||
| errBodySizeLimit = errors.New("body size limit exceeded") | ||
| UserAgent = fmt.Sprintf("%s/%s", PluginName, ScraperVersion) |
There was a problem hiding this comment.
Can we change this to a constant?
Also, it looks slightly weird to see vars in a file named const.go 😛
There was a problem hiding this comment.
Haha, sure I'll re-organize no worries.
server/plugin.go
Outdated
| @@ -113,13 +120,42 @@ func (p *Plugin) OnActivate() error { | |||
| return mutateSampleLabels(l, target, *p.configuration.HonorTimestamps, []*relabel.Config{}) | |||
| } | |||
|
|
|||
| ticker := time.Tick(time.Duration(scrapeInterval) * time.Second) | |||
There was a problem hiding this comment.
I'd recommend to use time.Ticker instead and explicitly stop it when no longer needed.
server/plugin.go
Outdated
| for { | ||
| select { | ||
| case <-p.closeChan: | ||
| return |
There was a problem hiding this comment.
We need to signal until this statement has hit. When you run close(ch), it does not guarantee that the receiving end will have received the event. So we need something similar to the websocket closing mechanism. Either another channel, or a sync.WaitGroup.
So on the caller side:
close(ch)
<-closedcallee side
case <-p.closeChan:
closed <- struct{}{}
returnOr the same can be done via a WaitGroup as well. 0/5.
There was a problem hiding this comment.
I think we can ditch this if we use time.Ticker and close that while exiting.
server/plugin.go
Outdated
| case <-p.closeChan: | ||
| return | ||
| case <-ticker: | ||
| cfg, err := p.configuration.Clone() |
There was a problem hiding this comment.
This can potentially be costly. Every Clone is a json Marshal/Unmarshal. Wondering if we can ensure to not modify the config in the scrapeFn? It seems like we are simply reading some config fields in there.
server/plugin.go
Outdated
| _ = level.Error(logger).Log("msg", "configuration could not be cloned", "err", err) | ||
| break | ||
| } | ||
| buf := new(bytes.Buffer) |
There was a problem hiding this comment.
Would suggest to move the creation outside the loop. And then reset the buffer (buf.Reset) for every scrape. This way we reduce GC.
server/plugin.go
Outdated
| @@ -91,10 +97,11 @@ func (p *Plugin) OnActivate() error { | |||
| if host == "" { | |||
| host = "localhost" | |||
| } | |||
| targetAddres := host + ":" + port | |||
server/scrape.go
Outdated
| if *conf.BodySizeLimitBytes <= 0 { | ||
| conf.BodySizeLimitBytes = model.NewInt64(math.MaxInt64) | ||
| } |
There was a problem hiding this comment.
Well, not a likely case TBH.
server/scrape.go
Outdated
| if n >= *conf.BodySizeLimitBytes { | ||
| return "", errBodySizeLimit | ||
| } |
There was a problem hiding this comment.
Do we need this check when the LimitReader guarantees this? Seems like we are testing the standard library here.
There was a problem hiding this comment.
Right. Removing. But LimitReader won't return an error, it would just read the limit we have set.
There was a problem hiding this comment.
It does return an io.EOF error: https://pkg.go.dev/io#LimitReader. It's just that it isn't very indicative of what happened. I'd say let's start with this for now. We can move to counting the bytes read in case there's a need.
server/scrape.go
Outdated
| return "", err | ||
| } | ||
|
|
||
| n, err := io.Copy(w, io.LimitReader(gzipr, *conf.BodySizeLimitBytes)) |
There was a problem hiding this comment.
It seems like a cleaner way would be to detect the encoding, and then wrap the reader in a gzip reader in an if condition, and then do the io.Copy as the common code outside the if condition. Looks like that would reduce a lot of the repetition here.
There was a problem hiding this comment.
Makes much sense, updated accordingly.
Summary
Add the scraper logic, for now it just scrapes but does not writes to the tsdb, that will be taken care of with the https://mattermost.atlassian.net/browse/MM-55226
Ticket Link
https://mattermost.atlassian.net/browse/MM-55225