-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Improved metrics setup #434
base: main
Are you sure you want to change the base?
Conversation
c4422a8
to
3aa2723
Compare
78ef6d5
to
5e97bf6
Compare
d264342
to
79bfb06
Compare
5e97bf6
to
7974730
Compare
3504563
to
0c1cbe2
Compare
e0b7ea2
to
2baf280
Compare
3a4e5b3
to
2b33af6
Compare
c113094
to
db8bd4b
Compare
2b33af6
to
dee8cd9
Compare
db8bd4b
to
a852e96
Compare
1448895
to
c8ea8db
Compare
a852e96
to
45c1c89
Compare
2af0abf
to
ef50ba6
Compare
ef50ba6
to
aced0c0
Compare
49055cc
to
711da4f
Compare
|
||
// WithTags sets the tags that are sent with every metric, shorthand for | ||
// statsd.WithTags() | ||
func WithTags(tags ...string) Option { |
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.
I think we shouldn't allow clients of this lib to remove the default tags (environment:*
, service:*
and version:*
), especially since the lib cannot be initiailized if env vars for those tags are missing. Maybe change the func name to WithExtraTags()
and append passed tags to options.tags
instead of replacing them?
func WithExtraTags(tags ...string) Option {
return func(options *options) error {
options.tags = append(options.tags, tags...)
return nil
}
}
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.
I agree the default ones should not be removed. I prefer the name WithTags
. I'll ensure the default ones are not removed when setting tags. I want having to support repetitive calls to the function as well.
Closes: #433