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

Convert remaining collectors to use ConstMetrics #389

Merged
merged 1 commit into from
Jan 3, 2017

Conversation

discordianfish
Copy link
Member

I didn't touch the gmond and megacli collectors since we'll remove them soon anyway.
I also kept devError a regular metric because that seemed appropriate to me since it's about counting all errors that happened in the exporter, not gathering some external state where ConstMetrics should be used.

Closes #240

c.metric.Set(now)
c.metric.Collect(ch)
return err
ch <- prometheus.MustNewConstMetric(c.desc, prometheus.CounterValue, now)
Copy link
Contributor

Choose a reason for hiding this comment

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

Gauge

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a counter before.. But yeah I remember we discussed that timestamps should be gauges. Will change.

}, nil
}

func (c *timeCollector) Update(ch chan<- prometheus.Metric) (err error) {
func (c *timeCollector) Update(ch chan<- prometheus.Metric) error {
now := float64(time.Now().Unix())
log.Debugf("Set time: %f", now)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs updating

@@ -61,8 +49,7 @@ func (c *loadavgCollector) Update(ch chan<- prometheus.Metric) (err error) {
}
for i, load := range loads {
log.Debugf("Set load %d: %f", i, load)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs updating

Copy link
Member Author

Choose a reason for hiding this comment

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

What needs updating? You want me to remove that debug line?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's no longer "Set"

}
}
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go

prometheus.NewDesc("node_ipvs_backend_weight", "The current backend weight by local and remote address.", []string{"local_address", "local_port", "remote_address", "remote_port", "proto"}, nil).String(): (<-sink).Desc().String(),
} {
if expected != got {
log.Fatalf("Expected '%s' but got '%s'", expected, got)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be using t rather than log

@@ -40,7 +40,7 @@ var (

type diskstatsCollector struct {
ignoredDevicesPattern *regexp.Regexp
metrics []prometheus.Collector
metrics []typedDesc
Copy link
Contributor

Choose a reason for hiding this comment

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

descs would be clearer

@mdlayher
Copy link
Contributor

Amazing work! 👏

@discordianfish
Copy link
Member Author

@brian-brazil Fixed your comments
@SuperQ Can you have a look as well?

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Awesome :shipit:

@discordianfish
Copy link
Member Author

Given that this is straight forward and 1:1 what we discussed in #240, I merge this with just @SuperQ's review. Hope that's fine.

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.

4 participants