Change default metrics namespace separator from colon to underscore #43
Conversation
|
This is required by jaegertracing/jaeger#776 |
Codecov Report
@@ Coverage Diff @@
## master #43 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 25 25
Lines 710 719 +9
=====================================
+ Hits 710 719 +9
Continue to review full report at Codecov.
|
|
I'm marking this as a WIP until jaegertracing/jaeger#776 is reviewed. |
|
@yurishkuro I remember discussing this PR last Friday, but don't remember the outcome: is this PR OK as it is? |
jpkrohling
force-pushed
the
732-MetricNames
branch
3 times, most recently
from
922bd0d
to
0fd7921
Compare
| "my-counter": 6, | ||
| "other-counter": 8, | ||
| "namespace.my-counter|x=y": 7, | ||
| "ns.subns|service=a-service": 9, |
There was a problem hiding this comment.
The current master would return this metric as ns.subns.|service=a-service (extra dot at the end, before the |)
|
|
||
| if name == "" { | ||
| return l.namespace | ||
| } |
There was a problem hiding this comment.
+1
NB: this kind of code is repeated several times in this library, it could use some refactoring.
metrics/prometheus/factory.go
Outdated
| @@ -184,7 +184,7 @@ func (f *Factory) subScope(name string) string { | |||
| if name == "" { | |||
| return f.normalize(f.scope) | |||
| } | |||
| return f.normalize(f.scope + ":" + name) | |||
| return f.normalize(f.scope + "_" + name) | |||
There was a problem hiding this comment.
@jpkrohling we had an internal discussion, the proposal is that instead of hardcoding this separator we make it an option to the factory (since it already supports options). Then in jaeger we default to _ (without bothering to make it a cli switch), but in jaeger-client-go people can, if necessary, instantiate the factory with : as before.
There was a problem hiding this comment.
I just updated the PR to include this suggestion. I decided to use a rune for the separator, as I think a single char should suffice, but let me know if a string would be more suitable.
metrics/prometheus/factory.go
Outdated
| @@ -57,6 +59,14 @@ func WithBuckets(buckets []float64) Option { | |||
| } | |||
| } | |||
|
|
|||
| // WithSeparator returns an option that sets the default separator for the namespace | |||
| // If not used, we fallback to underscore | |||
There was a problem hiding this comment.
- nit: end sentences with dots.
- underscore and colon are the only valid choices for a separator for Prometheus. Given that, another option would be to use an enum (which is better than simply documenting valid values).
type Separator rune
const (
SeparatorUnderscore rune = '_'
SeparatorColon = ':'
)There was a problem hiding this comment.
I assume the type of SeparatorUnderscore is Separator instead of rune?
jpkrohling
force-pushed
the
732-MetricNames
branch
2 times, most recently
from
845576f
to
9f8c612
Compare
|
The commits have been squashed and the PR updated with the latest suggestion (enum for separator values) |
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
force-pushed
the
732-MetricNames
branch
from
9f8c612
to
631dd05
Compare
jpkrohling
changed the title
yurishkuro
changed the title
|
@jpkrohling released as 1.5.0 |
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de