Skip to content

feat: export inner MetricVecBuilder structs#490

Open
MrCroxx wants to merge 2 commits into
tikv:masterfrom
MrCroxx:xx/export-builder-v2
Open

feat: export inner MetricVecBuilder structs#490
MrCroxx wants to merge 2 commits into
tikv:masterfrom
MrCroxx:xx/export-builder-v2

Conversation

@MrCroxx

@MrCroxx MrCroxx commented May 11, 2023

Copy link
Copy Markdown
Member

Signed-off-by: MrCroxx mrcroxx@outlook.com

This PR exports MetricVecBuilder types, with which users can easily build customized tools.

Signed-off-by: MrCroxx <mrcroxx@outlook.com>
@MrCroxx

MrCroxx commented May 11, 2023

Copy link
Copy Markdown
Member Author

In risingwave#9756 I built self-managed metrics generic types with the inner builder types. IMO exporting the inner builder helps a lot. Is It okay to export them to all user or is there any better way to achieve it? Thanks a lot. 🙏

@TennyZhuang

Copy link
Copy Markdown

You should also bump the MSRV in the CI file.

@TennyZhuang TennyZhuang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rest LGTM

@MrCroxx

MrCroxx commented May 12, 2023

Copy link
Copy Markdown
Member Author

Seems a new version of indirect dependency requires the new rust version. 🤔

@MrCroxx

MrCroxx commented May 12, 2023

Copy link
Copy Markdown
Member Author

@nickelc PTAL 🙏 If this PR is reasonable. Thanks a lot.

@Connor1996 Connor1996 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@MrCroxx

MrCroxx commented May 15, 2023

Copy link
Copy Markdown
Member Author

@lucab PTAL

@nickelc

nickelc commented May 15, 2023

Copy link
Copy Markdown
Contributor

The changes look fine.

Seems a new version of indirect dependency requires the new rust version. thinking

The dev dependency rayon 1.7 requires Rust 1.59 and the indirect dependency
security-framework v2.9.0 now requires Rust 1.60.

$ cargo tree --target all -i security-framework --features push
security-framework v2.9.0
└── native-tls v0.2.11
    ├── hyper-tls v0.5.0
    │   └── reqwest v0.11.17
    │       └── prometheus v0.13.3 (/projects/various/rust-prometheus)
    ├── reqwest v0.11.17 (*)
    └── tokio-native-tls v0.3.1
        ├── hyper-tls v0.5.0 (*)
        └── reqwest v0.11.17 (*)

@MrCroxx

MrCroxx commented May 17, 2023

Copy link
Copy Markdown
Member Author

@nickelc Thanks for help. Should I pin the dependency to the version that meets the MSRV(1.57) requires or update MSRV to a higher version?

@lucab

lucab commented May 17, 2023

Copy link
Copy Markdown
Member

@MrCroxx thanks for the patch! Let's bump the MSRV, 1.60 sounds like a good target.
Could you please do that in a separate PR? I'll review and merge that one, so that we can first unblock CI and then get to the rest of changes later.

@MrCroxx

MrCroxx commented May 17, 2023

Copy link
Copy Markdown
Member Author

@nickelc Could you please do that in a separate PR? I'll review and merge that one, so that we can first unblock CI and then get to the rest of changes later.

Sure~

Done with #491 .

@MrCroxx

MrCroxx commented May 17, 2023

Copy link
Copy Markdown
Member Author

Besides, would you like to publish a new version of this lib after this PR is merged?

@MrCroxx

MrCroxx commented May 18, 2023

Copy link
Copy Markdown
Member Author

@nickelc Sorry to bother you again. Can this PR be merged?

@MrCroxx

MrCroxx commented Sep 6, 2023

Copy link
Copy Markdown
Member Author

@lucab Hi, any updates? Can this PR be merged?

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.

5 participants