-
Notifications
You must be signed in to change notification settings - Fork 79
Remove mandatory opencensus_task label from node and respect metric prefix #184
Remove mandatory opencensus_task label from node and respect metric prefix #184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the second commit opencensus_task is not replaced. Should the title be changed then?
metrics_proto.go
Outdated
func (se *statsExporter) metricTypeFromProto(name string) string { | ||
prefix := se.o.MetricPrefix | ||
if prefix != "" { | ||
if !strings.HasSuffix(prefix, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check can be done at initialization of the exporter because it is not changing (se.o.MetricPrefix) and avoid doing it all the time. Also if empty can be set to the default value, so this function would become:
se.metricPrefix is pre-calculated.
func (se *statsExporter) metricTypeFromProto(name string) string {
if !hasDomain(name) {
name = se.metricPrefix + name
}
return name
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check can be done at initialization of the exporter because it is not changing (se.o.MetricPrefix) and avoid doing it all the time.
Moved to initialization.
Also if empty can be set to the default value
This is true but there are cases when prefix is not empty but we still need to apply the custom.googleapis.com/opencensus/
domain. For example see https://github.com/census-ecosystem/opencensus-go-exporter-stackdriver/pull/184/files#diff-4b2a059ff46795cb078274ba257d81a7R768.
// hasDomain checks if the metric name already has a domain in it. | ||
func hasDomain(name string) bool { | ||
for _, domain := range domains { | ||
if strings.Contains(name, domain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HasPrefix probably?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, because domains can have different prefixes e.g appengine.googleapis.com
, container.googleapis.com
, etc.
fbf8a86
to
40caee4
Compare
abbddc0
to
cf3aad7
Compare
No description provided.