-
Notifications
You must be signed in to change notification settings - Fork 128
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
export version as metric #320
export version as metric #320
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.
LGTM in general, but I find some points weird.
|
||
var registerLock = sync.Once{} | ||
|
||
// serviceNameToLabelFormat convert service name to lower case and remove all '_' |
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.
IMO, translating Acra-Server
into acra_server
is more readable. But I guess the idea behind removing the dashes is to keep the service name as one word so that it's more easily distinguishable in labels?
There's also typo in the comment (_
→ -
), we replace dashes, not underscores.
// serviceNameToLabelFormat convert service name to lower case and remove all '_' | |
// serviceNameToLabelFormat convert service name to lower case and remove all '-' |
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.
@Lagovas is this renaming crucial?
I would rather not introduce the third type of naming (currently we have acra-server
as service name, and binary name etc, and AcraServer
for human-friendly logs and texts).
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.
all our metrics have next naming: acraconnector_*
/acraserver_*
/acratranslator_*
according to our discussion in T662
this function was added to automatically convert serviceName to correct metric's format
} | ||
|
||
// MajorAsFloat64 return major number as float64 | ||
func (v *Version) MajorAsFloat64() (float64, error) { |
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.
Version components represented as real numbers look very weird to me. Using strings for components storage is weird as well.
I think it would be more natural to store ints
in Version
. We validate them as integers in GetParsedVersion
after all. Then we can cast them as float64
only in ExportVersionMetric
as using real numbers looks like an implementation detail of gauges to me.
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.
version used only in two places: as metrics where they exported as float64 according to prometheus client interface and as string in log message at the start
as is from VERSION
variable. Storing as string in VERSION
var allow override it at compilation time with command go build -ldflags '-X github.com/cossacklabs/acra/utils.VERSION=<my_new_version>'
via deploying tools with hash of commit or other build's metadata. Golang allow override only string values, so I decided to store it as a string and parse into the structure for metrics or other usages.
in a future version may be extended with pre-release part or with build metadata as it allowed in semver specification. and I thought that it is okay to store as strings and provide getters for specific types if necessary.
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.
Overall LGTM, let's just solve naming thing
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.
Thanks!
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.
👍
<service_name>_version_major
,<service_name>_version_minor
,<service_name>_version_patch
metrics where <service_name> replaced with acraserver | acraconnector | acratranslator (all services that works as daemon)