Skip to content
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

chore: Move VersionCollector to collectors directory #1427

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

ArthurSens
Copy link
Member

Moving the newly added Version Collector to the collectors' directory. It's being kept in a separate package because, as @SuperQ mentioned, it uses init() to collect some information. We don't want to do that unless it is explicit to importers

I also added an example in a separate commit, happy to move it to a separate PR if that's preferred.

@ArthurSens ArthurSens force-pushed the move-versioncollector branch from d7cbb71 to bd1d5af Compare January 9, 2024 00:14
@SuperQ
Copy link
Member

SuperQ commented Jan 9, 2024

I don't think this package belongs in the collectors directory because the primary use case of this package is to provide a structured version information. The version metric is an extra feature, not the primary feature.

Honestly, I think we were wrong in moving this to client_golang. The version information package probably makes more sense in exporter-toolkit.

@SuperQ SuperQ requested a review from roidelapluie January 9, 2024 10:11
@SuperQ
Copy link
Member

SuperQ commented Jan 11, 2024

As a counter-proposal, we could keep version in common. But move the NewCollector() function here.

In order to do this, I've opened prometheus/common#563 to make the helper function public.

@ArthurSens ArthurSens force-pushed the move-versioncollector branch from bd1d5af to 8a3bec2 Compare January 11, 2024 23:24
@ArthurSens
Copy link
Member Author

I've changed the PR to continue using prometheus/common, but still depends on prometheus/common#565

@ArthurSens ArthurSens force-pushed the move-versioncollector branch 3 times, most recently from c1fb44d to 7cce798 Compare January 12, 2024 21:39
Arthur Silva Sens added 2 commits January 14, 2024 16:05
@ArthurSens ArthurSens force-pushed the move-versioncollector branch from 7cce798 to 7cf8416 Compare January 14, 2024 19:05
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

@kakkoyun kakkoyun merged commit 38631c6 into main Jan 15, 2024
9 checks passed
@kakkoyun kakkoyun deleted the move-versioncollector branch January 15, 2024 09:07
@bwplotka
Copy link
Member

💪🏽

@SuperQ
Copy link
Member

SuperQ commented Jan 15, 2024

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants