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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove drand_version metric #945

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Remove drand_version metric #945

merged 1 commit into from
Mar 30, 2022

Conversation

mcamou
Copy link
Contributor

@mcamou mcamou commented Mar 30, 2022

No description provided.

@mcamou mcamou requested a review from AnomalRoil March 30, 2022 13:05
@mcamou mcamou force-pushed the metrics branch 2 times, most recently from 2de8faa to e947938 Compare March 30, 2022 13:07
@mcamou mcamou changed the title Remove drand_version metric, put the version as a label in drand_build_time Remove drand_version metric Mar 30, 2022
@mcamou
Copy link
Contributor Author

mcamou commented Mar 30, 2022

After creating the dashboard I realized that we don't even need to convert the version to a number if we add it as a label in drand_build_time, which also makes the dashboard nicer.

@mcamou mcamou added the monitoring Area: Monitoring label Mar 30, 2022
@mcamou
Copy link
Contributor Author

mcamou commented Mar 30, 2022

Related: #939

ConstLabels: map[string]string{"build": common.COMMIT},
}, func() float64 { return float64(getVersionNum(common.GetAppVersion())) })
// DKGStateChangeTimestamp tracks DKG status changes
DKGStateChangeTimestamp = prometheus.NewGaugeVec(prometheus.GaugeOpts{
Copy link
Member

Choose a reason for hiding this comment

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

is it expected that nothing is going to report this metric?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ARGH. I'd stashed that before the first commit then unstashed and forgot to restash before the second one. It's part of the next thing to implement. I've removed it and force-pushed.

@mcamou mcamou force-pushed the metrics branch 2 times, most recently from 66b49b5 to 8f5c689 Compare March 30, 2022 14:30
@mcamou mcamou merged commit d823174 into master Mar 30, 2022
@mcamou mcamou deleted the metrics branch March 30, 2022 14:46
Comment on lines 155 to 157
DrandBuildTime = prometheus.NewUntypedFunc(prometheus.UntypedOpts{
Name: "drand_build_time",
Help: "Timestamp when the binary was built in seconds since the Epoch",
Copy link
Member

Choose a reason for hiding this comment

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

This might need to be rephrased / renamed if it's also responsible for the version, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The metric name refers to the meaning of the value, which is still the build time. We're using labels to pass additional info, but their meaning is in the label name, not in the metric name.

@yiannisbot yiannisbot added this to the Ceremony dashboard milestone Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monitoring Area: Monitoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants