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.

Sign up for GitHub

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

Jump to bottom

Rolling summary configuration #444

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Rolling summary configuration #444

merged 2 commits into from
Mar 1, 2024

Conversation

cparratto
Copy link
Contributor

Adds bucket duration as configurable element for rolling summaries.

@cparratto cparratto force-pushed the prom-rolling-summary-buckets branch from f062d8d to 739d6ba Compare February 7, 2024 22:24
@cparratto
Copy link
Contributor Author

I would like to be able to configure these values. Happy to make any other changes required to get this upstream

@cparratto cparratto mentioned this pull request Feb 8, 2024
Closed
@tobz
Copy link
Member

tobz commented Feb 11, 2024

I'll take a look at this in the next day or two.

tobz
tobz requested changes Feb 13, 2024
View reviewed changes
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

If we're going to expose the configuration of rolling summaries, we need to expose both the number of buckets as well as the bucket width.

@cparratto
Copy link
Contributor Author

If we're going to expose the configuration of rolling summaries, we need to expose both the number of buckets as well as the bucket width.

Sounds good. I will add this to the PR now.

@cparratto cparratto force-pushed the prom-rolling-summary-buckets branch from bd25e22 to 640663e Compare February 14, 2024 17:14
@cparratto cparratto requested a review from tobz February 14, 2024 17:15
tobz
tobz requested changes Feb 15, 2024
View reviewed changes
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Thanks for adding the change to update the bucket count, too.

Left some feedback just to clean things up.

metrics-exporter-prometheus/src/distribution.rs Outdated Show resolved Hide resolved
metrics-exporter-prometheus/src/builder.rs Outdated Show resolved Hide resolved
metrics-exporter-prometheus/src/distribution.rs Outdated Show resolved Hide resolved
@cparratto cparratto requested a review from tobz February 26, 2024 16:59
@cparratto cparratto force-pushed the prom-rolling-summary-buckets branch from f4b700a to e869445 Compare February 28, 2024 16:43
tobz
tobz approved these changes Mar 1, 2024
View reviewed changes
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Looks like there's some Clippy lints, but overall, this looks good enough to me to merge. I'll deal with the lints as part of some other changes I need to make in the same area.

@tobz tobz merged commit a2c5a8a into metrics-rs:main Mar 1, 2024
11 of 12 checks passed
@tobz tobz added C-exporter Component: exporters such as Prometheus, TCP, etc. E-simple Effort: simple. T-enhancement Type: enhancement. S-awaiting-release Status: awaiting a release to be considered fixed/implemented. labels Mar 1, 2024
mnpw pushed a commit to mnpw/metrics that referenced this pull request Mar 8, 2024
@cparratto @mnpw
Rolling summary configuration (metrics-rs#444)
7611322
@lilyball lilyball mentioned this pull request Mar 15, 2024
Closed
@tobz
Copy link
Member

tobz commented Mar 16, 2024

Released in [email protected].

Thanks again for your contribution!

@tobz tobz removed the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobz tobz tobz approved these changes

Assignees
No one assigned
Labels
C-exporter Component: exporters such as Prometheus, TCP, etc. E-simple Effort: simple. T-enhancement Type: enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cparratto @tobz