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

Treat custom textfile metric timestamps as errors #769

Merged
merged 1 commit into from
Feb 27, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions collector/fixtures/textfile/client_side_timestamp.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# HELP node_textfile_scrape_error 1 if there was an error opening or reading a file, 0 otherwise
# TYPE node_textfile_scrape_error gauge
node_textfile_scrape_error 1
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
metric_with_custom_timestamp 1 1441205977284
normal_metric 2
4 changes: 2 additions & 2 deletions collector/fixtures/textfile/two_metric_files/metrics2.prom
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
testmetric2_1{foo="bar"} 30 1441205977284
testmetric2_2{foo="baz"} 40 1441205977284
testmetric2_1{foo="bar"} 30
testmetric2_2{foo="baz"} 40
25 changes: 19 additions & 6 deletions collector/textfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,36 +191,49 @@ func (c *textFileCollector) Update(ch chan<- prometheus.Metric) error {
// Iterate over files and accumulate their metrics.
files, err := ioutil.ReadDir(c.path)
if err != nil && c.path != "" {
log.Errorf("Error reading textfile collector directory %s: %s", c.path, err)
log.Errorf("Error reading textfile collector directory %q: %s", c.path, err)
error = 1.0
}

fileLoop:
for _, f := range files {
if !strings.HasSuffix(f.Name(), ".prom") {
continue
}
path := filepath.Join(c.path, f.Name())
file, err := os.Open(path)
if err != nil {
log.Errorf("Error opening %s: %v", path, err)
log.Errorf("Error opening %q: %v", path, err)
error = 1.0
continue
}
var parser expfmt.TextParser
parsedFamilies, err := parser.TextToMetricFamilies(file)
file.Close()
if err != nil {
log.Errorf("Error parsing %s: %v", path, err)
log.Errorf("Error parsing %q: %v", path, err)
error = 1.0
continue
}
// Only set this once it has been parsed, so that
// a failure does not appear fresh.
mtimes[f.Name()] = f.ModTime()
for _, mf := range parsedFamilies {
for _, m := range mf.Metric {
if m.TimestampMs != nil {
log.Errorf("Textfile %q contains unsupported client-side timestamps, skipping entire file", path)
error = 1.0
continue fileLoop
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of continue-to-label. What about moving checking of parsedFamilies to a function this is called here so you can continue without label?

Copy link
Member

Choose a reason for hiding this comment

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

+1, it would be nice to avoid this.

Copy link
Member

Choose a reason for hiding this comment

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

@discordianfish Can we merge this and fix it later?

Copy link
Member

Choose a reason for hiding this comment

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

Hrm I don't like this. But if you prefer, it's okay with me but let's open an issue for this.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, I would like to get 0.16 out asap, and this is worth getting into 0.16.

}
}
if mf.Help == nil {
help := fmt.Sprintf("Metric read from %s", path)
mf.Help = &help
}
}

// Only set this once it has been parsed and validated, so that
// a failure does not appear fresh.
mtimes[f.Name()] = f.ModTime()

for _, mf := range parsedFamilies {
convertMetricFamily(mf, ch)
}
}
Expand Down
4 changes: 4 additions & 0 deletions collector/textfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ func TestTextfileCollector(t *testing.T) {
path: "fixtures/textfile/nonexistent_path",
out: "fixtures/textfile/nonexistent_path.out",
},
{
path: "fixtures/textfile/client_side_timestamp",
out: "fixtures/textfile/client_side_timestamp.out",
},
{
path: "fixtures/textfile/different_metric_types",
out: "fixtures/textfile/different_metric_types.out",
Expand Down