-
Notifications
You must be signed in to change notification settings - Fork 3
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow empty namespace, namespace chain for nested structs #34
base: master
Are you sure you want to change the base?
Conversation
@carrascoacd @juananrey, review it please. |
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.
It looks good to me. Thanks for the contribution!
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.
The approach LGTM, thought I'd keep behavior of public Init function as it is. Please also update the CHANGELOG.md
@@ -100,8 +106,12 @@ func (in initializer) initMetrics(group reflect.Value, namespaces ...string) err | |||
} | |||
} else if fieldType.Type.Kind() == reflect.Struct { | |||
namespace, ok := fieldType.Tag.Lookup("namespace") |
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.
WDYT if we write whole struct branch like this:
newNamespaces := namespaces
namespaceTag, ok := fieldType.Tag.Lookup("namespace")
if ok && namespaceTag != "" {
newNamespaces = append(newNamespaces, namespaceTag)
}
if err := in.initMetrics(field, newNamespaces...); err != nil {
return err
}
var namespaces []string | ||
|
||
if namespace != "" { | ||
namespaces = append(namespaces, namespace) | ||
} | ||
|
||
return in.initMetrics(metricsPtr.Elem(), namespaces...) |
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.
These changes are not necessary to make namespace
optional for nested structs, right?
Also. This is kind of breaking change. Before the change call to Init with empty namespace was producing _blah_blah metric (note the starting underscore). Even though it was not intentional, I'd keep this behaviour
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.
Underscores are added only for nested structures.
For example:
type metrics struct {
AAA func() prometheus.Counter `name:"aaa" help:""`
Base struct {
CCC func() prometheus.Counter `name:"ccc" help:""`
}
Sub struct {
BBB func() prometheus.Counter `name:"bbb" help:""`
} `namespace:"sub"`
}
This will produce aaa
, ccc
, and _sub_bbb
metrics. This looks a bit strange.
How about replacing the namespace
argument with namespaces ...string
in the Init
function?
This would allow metrics to be created without a prefix at all while preserving this behavior.
e6b62bd
to
274ab3a
Compare
The namespace is optional.
Nested structs without a namespace are useful for embedded structs.