-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
bugfix: Initialize metrics with empty tags #2847
bugfix: Initialize metrics with empty tags #2847
Conversation
Workspace string | ||
} | ||
|
||
func (s ProjectScopeTags) Loadtags() map[string]string { |
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.
Should this be named like this?
func (s ProjectScopeTags) Loadtags() map[string]string { | |
func (s ProjectScopeTags) loadTags() map[string]string { |
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.
Unfortunately I think we have to keep this public since it is used on server/events/instrumented_project_command_runner.go
, maybe the function name choice wasn't clear enough?
I guess that to make this private I could make an ScopeTags
interface on the metrics package, and call the loadTags
from there. Then ProjectScopeTags
would be an implementation of that interface passed as argument to a function that load the tags on the scope. I'm not so sure this would work though, but I can give a try.
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.
That's ok, this was a nitpick.
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.
Looks good. Left one nitpick.
what
Added empty tags to metrics initialization to avoid errors when running commands.
Tried to hide the implementation behind the
ProjectScopeTags
so we can have an easy way to implement empty tags like this:Instead of this:
This should ensure that in the future if someone adds new tags to the
ProjectScopeTags
they still will be initialized empty while still can use theSetProjectScopeTags
to add the values.why
references