-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add version package #1422
Add version package #1422
Conversation
54d489a
to
96aedfd
Compare
Migrate the `version` package from `github.com/prometheus/common` to `github.com/prometheus/client_golang` in order to break the circular dependency. * Make `version` a top level package because it uses `init()` to populate data. Related to: prometheus/common#58 Signed-off-by: SuperQ <[email protected]>
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, just I would put it prometheus/collectors/version to group collectors in some way.
In our client_golang sync, with @ArthurSens and @kakkoyun we decided at the end to move it to Sorry for confusion. 🤗 |
I don't think that's the right place for it. The version package is mostly about having a structured version feature, the metrics collector is secondary. |
Yes, I understand, but from the client_golang point of view it's generally metric collector. To use it you have provide that structure, which you might reuse in other places, but for this repo, this is the best place. One could provide this structure on their own and somewhere else, but it's too simple to split version structure AND collector separately, thus collectors directory might be a fair trade-off (: I feel it's Sorry I indicated "fair" on Slack on your point around long URL, it's some argument but indeed pretty weak (was late for me) 🙈 |
Migrate the
version
package fromjackfan.us.kg/prometheus/common
tojackfan.us.kg/prometheus/client_golang
in order to break the circular dependency.version
a top level package because it usesinit()
to populate data.Related to: prometheus/common#58