Skip to content

Conversation

chardch
Copy link

@chardch chardch commented Apr 17, 2025

Addresses #16371

@chardch
Copy link
Author

chardch commented Apr 17, 2025

@krajorama could you help review this PR that addresses #16371?

@chardch chardch force-pushed the global-always-scrape-classic-hist branch from 5090930 to f72f39d Compare April 21, 2025 17:48
@krajorama krajorama self-requested a review April 22, 2025 06:46
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

LGTM, CI has flagged some missing/extra white spaces in the test config files, please fix those

@chardch chardch force-pushed the global-always-scrape-classic-hist branch from f72f39d to 29a6ae6 Compare April 23, 2025 16:45
@chardch
Copy link
Author

chardch commented Apr 23, 2025

LGTM, CI has flagged some missing/extra white spaces in the test config files, please fix those

Thank you! Fixed those white space issues

@roidelapluie
Copy link
Member

We are missing documentation here.

Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

@roidelapluie is right, needs documentation.md update, please see #16226 for reference.

@chardch chardch force-pushed the global-always-scrape-classic-hist branch from 29a6ae6 to caa2efd Compare May 1, 2025 16:19
@chardch
Copy link
Author

chardch commented May 1, 2025

Thanks @roidelapluie and @krajorama . I've updated the documentation for this

@chardch chardch requested a review from krajorama May 1, 2025 16:20
Addresses prometheus#16371
This will help with migrating to native histograms with `convert_classic_histograms_to_nhcb` since users may still need to keep the classic histograms during a migration

Signed-off-by: chardch <otwordsne@gmail.com>
@chardch chardch force-pushed the global-always-scrape-classic-hist branch from caa2efd to a1c157a Compare May 5, 2025 17:15
Copy link
Author

@chardch chardch left a comment

Choose a reason for hiding this comment

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

addressed requested change

Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

LGTM, I've built it locally and did some tests as well.

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