Skip to content

Apply the same logic to all metric type for Hostname prefixing #83

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Apply the same logic to all metric type for Hostname prefixing #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e07834
Apply the same logic to all metric type for Hostname prefixing
Sep 7, 2018
0b3f4e1
Prevent EnableHostname and EnableHostnameLabel to be true at the same…
Sep 7, 2018
36903e8
Add tests for hostname prefixing
Sep 24, 2018
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
24 changes: 18 additions & 6 deletions metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,12 @@ func (m *Metrics) IncrCounter(key []string, val float32) {
}

func (m *Metrics) IncrCounterWithLabels(key []string, val float32, labels []Label) {
if m.HostName != "" && m.EnableHostnameLabel {
labels = append(labels, Label{"host", m.HostName})
if m.HostName != "" {
if m.EnableHostnameLabel {
labels = append(labels, Label{"host", m.HostName})
} else if m.EnableHostname {
key = insert(0, m.HostName, key)
}
}
if m.EnableTypePrefix {
key = insert(0, "counter", key)
Expand All @@ -86,8 +90,12 @@ func (m *Metrics) AddSample(key []string, val float32) {
}

func (m *Metrics) AddSampleWithLabels(key []string, val float32, labels []Label) {
if m.HostName != "" && m.EnableHostnameLabel {
labels = append(labels, Label{"host", m.HostName})
if m.HostName != "" {
if m.EnableHostnameLabel {
labels = append(labels, Label{"host", m.HostName})
} else if m.EnableHostname {
key = insert(0, m.HostName, key)
}
}
if m.EnableTypePrefix {
key = insert(0, "sample", key)
Expand All @@ -111,8 +119,12 @@ func (m *Metrics) MeasureSince(key []string, start time.Time) {
}

func (m *Metrics) MeasureSinceWithLabels(key []string, start time.Time, labels []Label) {
if m.HostName != "" && m.EnableHostnameLabel {
labels = append(labels, Label{"host", m.HostName})
if m.HostName != "" {
if m.EnableHostnameLabel {
labels = append(labels, Label{"host", m.HostName})
} else if m.EnableHostname {
key = insert(0, m.HostName, key)
}
}
if m.EnableTypePrefix {
key = insert(0, "timer", key)
Expand Down
70 changes: 70 additions & 0 deletions metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package metrics
import (
"reflect"
"runtime"
"strings"
"testing"
"time"
)
Expand Down Expand Up @@ -376,6 +377,75 @@ func HasElem(s interface{}, elem interface{}) bool {

return false
}
func TestMetrics_HostnameBothProvided(t *testing.T) {
m := &MockSink{}
conf := DefaultConfig("")
conf.EnableHostnameLabel = true
conf.EnableHostname = true
if _, err := New(conf, m); err != nil && !strings.HasPrefix(err.Error(), "both EnableHostname") {
t.Fatalf("New should throw when given both EnableHostname and EnableHostnameLabel as true")
}
}

func TestMetrics_HostNameLabel(t *testing.T) {
m := &MockSink{}
conf := DefaultConfig("")
hostname := "testhostname"
conf.HostName = hostname
conf.EnableHostname = false
conf.EnableHostnameLabel = true
met, err := New(conf, m)
if err != nil {
t.Fatal(err)
}

key := []string{"test"}
met.SetGauge(append(key, "gauge"), 1)
met.IncrCounter(append(key, "counter"), 1)
met.AddSample(append(key, "summary"), 1)

hostnameLabel := Label{Name: "host", Value: hostname}

if len(m.keys) != 3 {
t.Fatalf("only 3 keys should be present")
}

for i, metric := range m.labels {
if !HasElem(metric, hostnameLabel) {
t.Fatalf("label hostname should be present and valid in %s. Labels: %+v", strings.Join(m.keys[i], "_"), metric)
}
}

}

func TestMetrics_HostName(t *testing.T) {
m := &MockSink{}
conf := DefaultConfig("")
hostname := "testhostname"
conf.HostName = hostname
conf.EnableHostname = true
conf.EnableHostnameLabel = false
met, err := New(conf, m)
if err != nil {
t.Fatal(err)
}

key := []string{"test"}
met.SetGauge(append(key, "gauge"), 1)
met.IncrCounter(append(key, "counter"), 1)
met.AddSample(append(key, "summary"), 1)

if len(m.keys) != 3 {
t.Fatalf("only 3 keys should be present")
}

for i, metric := range m.keys {
if !HasElem(metric, hostname) {
t.Fatalf("%s should be present in metric keys and valid in %s. Keys: %+v", hostname, strings.Join(m.keys[i], "_"), metric)
}
}

}

func TestMetrics_Filter_Whitelist(t *testing.T) {
m := &MockSink{}
Expand Down
6 changes: 6 additions & 0 deletions start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package metrics

import (
"fmt"
"os"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -69,6 +70,11 @@ func DefaultConfig(serviceName string) *Config {

// New is used to create a new instance of Metrics
func New(conf *Config, sink MetricSink) (*Metrics, error) {

if conf.EnableHostname && conf.EnableHostnameLabel {
return nil, fmt.Errorf("both EnableHostname and EnableHostnameLabel can't be true at the same time")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this risk being a breaking change for clients? Is it important the are mutually exclusive? I can't think of a very food reason to use both but If you are migrating between statsd and prometheus for example you might want both being reported in the interim?

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/armon/go-metrics/blob/master/statsd.go#L9 (not sure I linked properly)

Statsd and other sink handle labels on their own

In specific statsd case: append them in the name as a suffix, so if both were provided you could have
"localhost-something-localhost-label2

It could break clients using such configs initializers indeed but it will break the client interface anyway
This fix indeed fix the name of all the metrics except gauges if enableHostname is activated

I don't mind removing it tho: it was more of a safeguard against weird behaviors, what do you think ?

Copy link
Author

Choose a reason for hiding this comment

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

Since the metrics initialize is "mostly" done at start-up time for app, issues will be easier to detect for existing clients that does use both, compared to not seeing the same metrics name after a go get
but again, if you prefer, I really don't feel it's important to keep this if

met := &Metrics{}
met.Config = *conf
met.sink = sink
Expand Down