Skip to content

Allow empty namespace, namespace chain for nested structs #34

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 1 commit into
base: master
Choose a base branch
from
Open

Allow empty namespace, namespace chain for nested structs #34

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
18 changes: 14 additions & 4 deletions init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,13 @@ func (in initializer) Init(metrics interface{}, namespace string) error {
return fmt.Errorf("expected pointer to metrics struct, got %q", metricsPtr.Kind())
}

return in.initMetrics(metricsPtr.Elem(), namespace)
var namespaces []string

if namespace != "" {
namespaces = append(namespaces, namespace)
}

return in.initMetrics(metricsPtr.Elem(), namespaces...)
Comment on lines +85 to +91
Copy link

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

Copy link
Author

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.

Copy link

Choose a reason for hiding this comment

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

Hi! Sorry, for the late reply.

How about replacing the namespace argument with namespaces ...string in the Init function?

Oh, so you would like to initialze a metric without any namespace. What is your use case? I feel that having common prefix for related metrics is useful when querying. What you suggest is fine by me

Copy link
Author

Choose a reason for hiding this comment

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

What is your use case?

To be honest, I don’t remember why I needed this. xD

The official client allows this. I would like to keep this option and not impose any restrictions when using this package.

If you agree, I'll add ...string argument.

Copy link

Choose a reason for hiding this comment

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

If you agree, I'll add ...string argument.

Yes, this is fine by me

}

func (in initializer) initMetrics(group reflect.Value, namespaces ...string) error {
Expand All @@ -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")
Copy link

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
			}

if !ok {
return fmt.Errorf("field %s does not have the namespace tag defined", fieldType.Name)
if !ok || namespace == "" {
if err := in.initMetrics(field, namespaces...); err != nil {
return err
}

continue
}
if err := in.initMetrics(field, append(namespaces, namespace)...); err != nil {
return err
Expand Down Expand Up @@ -134,7 +144,7 @@ func (in initializer) initMetricFunc(field reflect.Value, structField reflect.St
// Validate the input of the metric function, it should have zero or one arguments
// If it has one argument, it should be a struct correctly tagged with label names
// If there are no input arguments, this metric will not have labels registered
var labelIndexes = make(map[label][]int)
labelIndexes := make(map[label][]int)
if fieldType.NumIn() > 1 {
return fmt.Errorf("field %s: expected 1 in arg, got %d", structField.Name, fieldType.NumIn())
} else if fieldType.NumIn() == 1 {
Expand Down
14 changes: 0 additions & 14 deletions init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,20 +86,6 @@ func TestInitializer_Init(t *testing.T) {
Something string
}{},
},
{
desc: "sub-struct doesn't define its namespace",
metrics: &struct {
Inner struct{}
}{},
},
{
desc: "sub-struct can't be initialized",
metrics: &struct {
Inner struct {
Deeper struct{} `thisdoesnothaveanamespace:"nothing"`
} `namespace:"thishasanamespace"`
}{},
},
{
desc: "non-exported field",
metrics: &struct {
Expand Down