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

Added basic file sd based on prom file sd #536

Closed
wants to merge 3 commits into from
Closed

Conversation

ivan-valkov
Copy link

ref: #492

Changes

Added basic file service discovery heavily based on prometheus' fileSD (https://github.com/prometheus/prometheus/blob/master/discovery/file/file.go)

The biggest difference is that we are not using target groups. That's why we cannot just use prometheus' fileSD. Instead, discovered files are represented in a struct that contains a list of strings.

The format of the files and therefore how we represent them is up for discussion. The current accepted format is just a json or yaml list of addresses.

Verification

Used the fileSD test from prometheus but modified it to not use target groups. (https://github.com/prometheus/prometheus/blob/master/discovery/file/file_test.go)

@ivan-valkov ivan-valkov self-assigned this Sep 25, 2018
@bwplotka
Copy link
Member

Looks like tests are legitimately failing (:

@xjewer
Copy link
Contributor

xjewer commented Sep 26, 2018

yeah, errcheck shouts someone forgot to check for returning errors, i.e. https://github.com/improbable-eng/thanos/pull/536/files#diff-022e915cc533082abfa05f04edaa963cR220

"github.com/improbable-eng/thanos/pkg/runutil"
"gopkg.in/yaml.v2"
"io/ioutil"
"os"
Copy link
Member

Choose a reason for hiding this comment

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

make format

func (d *FileDiscoverer) Run(ctx context.Context, ch chan<- *Discoverable) {
watcher, err := fsnotify.NewWatcher()
if err != nil {
level.Error(d.logger).Log("msg", "Error adding file watcher", "err", err)
Copy link
Member

Choose a reason for hiding this comment

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

This err is not really handled (:

}

// listFiles returns a list of all files that match the configured patterns.
func (d *FileDiscoverer) listFiles() []string {
Copy link
Member

Choose a reason for hiding this comment

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

s/listFiles/globFiles

for _, p := range d.paths {
files, err := filepath.Glob(p)
if err != nil {
level.Error(d.logger).Log("msg", "Error expanding glob", "glob", p, "err", err)
Copy link
Member

Choose a reason for hiding this comment

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

return error

// updated discoverables through the channel.
func (d *FileDiscoverer) refresh(ctx context.Context, ch chan<- *Discoverable) {
ref := map[string]struct{}{}
for _, p := range d.listFiles() {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add metrics to errors

for _, p := range d.listFiles() {
discoverable, err := d.readFile(p)
if err != nil {
level.Error(d.logger).Log("msg", "Error reading file", "path", p, "err", err)
Copy link
Member

Choose a reason for hiding this comment

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

Log lines should not captialised


case err := <-d.watcher.Errors:
if err != nil {
level.Error(d.logger).Log("msg", "Error watching file", "err", err)
Copy link
Member

Choose a reason for hiding this comment

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

captialised err

@ivan-valkov
Copy link
Author

agreed to reuse prometheus file sd instead of creating this one

@ivan-valkov ivan-valkov closed this Oct 9, 2018
@ivan-valkov ivan-valkov deleted the file-sd branch October 17, 2018 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants