Skip to content

metrics Gauge cleanup #2508

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.

Sign up for GitHub

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

Merged
brianolson merged 1 commit into from
Jul 27, 2021
Merged

metrics Gauge cleanup #2508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 21 additions & 35 deletions util/metrics/gauge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,25 @@ import (
"math"
"strconv"
"strings"
"time"

"github.com/algorand/go-deadlock"
)

// Gauge represent a single gauge variable.
type Gauge struct {
deadlock.Mutex
name string
description string
labels map[string]int // map each label ( i.e. httpErrorCode ) to an index.
valuesIndices map[int]*gaugeValues // maps each set of labels into a concrete gauge
}

type gaugeValues struct {
gauge float64
labels map[string]string
formattedLabels string
}

// MakeGauge create a new gauge with the provided name and description.
func MakeGauge(metric MetricName) *Gauge {
c := &Gauge{
Expand All @@ -35,15 +51,6 @@ func MakeGauge(metric MetricName) *Gauge {
return c
}

// TouchAll updates all the timestamps associated with this metric.
func (gauge *Gauge) TouchAll() {
gauge.Lock()
defer gauge.Unlock()
for _, val := range gauge.valuesIndices {
val.timestamp = time.Now()
}
}

// Register registers the gauge with the default/specific registry
func (gauge *Gauge) Register(reg *Registry) {
if reg == nil {
Expand Down Expand Up @@ -73,16 +80,14 @@ func (gauge *Gauge) Add(x float64, labels map[string]string) {
if gaugeObj, has := gauge.valuesIndices[labelIndex]; !has {
// we need to add a new gauge.
val := &gaugeValues{
gauge: x,
labels: labels,
timestamp: time.Now(),
gauge: x,
labels: labels,
}
val.createFormattedLabel()
gauge.valuesIndices[labelIndex] = val
} else {
// update existing value.
gaugeObj.gauge += x
gaugeObj.timestamp = time.Now()
}
}

Expand All @@ -97,16 +102,14 @@ func (gauge *Gauge) Set(x float64, labels map[string]string) {
if gaugeObj, has := gauge.valuesIndices[labelIndex]; !has {
// we need to add a new gauge.
val := &gaugeValues{
gauge: x,
labels: labels,
timestamp: time.Now(),
gauge: x,
labels: labels,
}
val.createFormattedLabel()
gauge.valuesIndices[labelIndex] = val
} else {
// update existing value.
gaugeObj.gauge = x
gaugeObj.timestamp = time.Now()
}
}

Expand Down Expand Up @@ -139,26 +142,11 @@ func (cv *gaugeValues) createFormattedLabel() {
cv.formattedLabels = buf.String()[1:]
}

// filterExpiredMetrics scans the gauge.valuesIndices map and removing all the gauges that
// haven't been updated in the past <maxMetricRetensionDuration> units of time.
func (gauge *Gauge) filterExpiredMetrics() {
Copy link
Copy Markdown
Contributor

@winder winder Jul 12, 2021

Choose a reason for hiding this comment

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

Removing this would be a problem if someone decides to use the label feature. It looks like that allows for creating dynamic guages based on user-defined keys. We don't currently use the dynamic labels feature in the networking code. In theory, someone might add a gauge by session ID (i.e. ping per connection). At that point removing this function would become be issue since it would grow unbounded.

A couple of possible solutions:

  1. Remove the labels feature (it seems to only be used by segment.go:EnterSegment and that function is not called anywhere).
  2. Return from this function immediately when len(valueIndices) <= 1
  3. Set maxMetricRetensionDuration to something very large in config.json

Copy link
Copy Markdown
Contributor Author

@brianolson brianolson Jul 12, 2021

Choose a reason for hiding this comment

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

MakeSegement EnterSegment LeaveSegment never occur in the code. It's completely unused and I'm tempted to delete segment.go entirely.
labels is also unused, I checked all uses of Gauge.Set() and labels is always nil.

Since there's no problem with the code as it is but only as it might be, I'd like to finish this bugfix and file a new issue for further util/metrics cleanup of dead code.

// find out what the cut off date is
metricRetensionThreshold := time.Now().Add(-maxMetricRetensionDuration)

for gaugeKey, gaugeObj := range gauge.valuesIndices {
if gaugeObj.timestamp.Before(metricRetensionThreshold) {
delete(gauge.valuesIndices, gaugeKey)
}
}
}

// WriteMetric writes the metric into the output stream
func (gauge *Gauge) WriteMetric(buf *strings.Builder, parentLabels string) {
gauge.Lock()
defer gauge.Unlock()

gauge.filterExpiredMetrics()

if len(gauge.valuesIndices) < 1 {
return
}
Expand Down Expand Up @@ -190,8 +178,6 @@ func (gauge *Gauge) AddMetric(values map[string]string) {
gauge.Lock()
defer gauge.Unlock()

gauge.filterExpiredMetrics()

if len(gauge.valuesIndices) < 1 {
return
}
Expand Down
39 changes: 0 additions & 39 deletions util/metrics/gaugeCommon.go
View file
Open in desktop

This file was deleted.