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

Additional tracking on histograms for means, std dev, p* quantile estimates #5897

Merged
merged 11 commits into from
Jun 24, 2024

Conversation

zeeshanlakhani
Copy link
Collaborator

Closes #4913.

This is part 1 of tracking more information within a histogram to compute min, max, means, std_dev, and estimates for p50, p90, and p99 and how to store/retrieve the associated info from Clickhouse.

@zeeshanlakhani zeeshanlakhani force-pushed the zl/extend-histogram branch 3 times, most recently from 1bcf5e2 to 4a3deff Compare June 14, 2024 11:16
Copy link
Collaborator

@bnaecker bnaecker 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 taking all this on, it's a big chunk of work! I've not been able to work through everything yet, but have some questions and comments around the guts of the P2 algorithm. Maybe we can take a pass at those, and then I'll work through the rest once we're happy? Please let me know if my questions aren't clear.

Thanks!

oximeter/oximeter/src/quantile.rs Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/test_util.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
@zeeshanlakhani zeeshanlakhani force-pushed the zl/extend-histogram branch 4 times, most recently from 9f90f94 to bf92d09 Compare June 19, 2024 05:17
@zeeshanlakhani zeeshanlakhani marked this pull request as ready for review June 19, 2024 05:17
@zeeshanlakhani zeeshanlakhani force-pushed the zl/extend-histogram branch 4 times, most recently from 8e9811e to 6ef6bc2 Compare June 21, 2024 15:21
Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

Looks great! I really like the new docstrings and doctests, and the cleanup of the quantile internals is all awesome. I've got a few remaining comments, most of which are pretty small at this point. Hopefully things are clear, but as always let me know if not! Thanks, this is really great to see.

oximeter/db/src/client/mod.rs Outdated Show resolved Hide resolved
oximeter/db/src/client/oxql.rs Outdated Show resolved Hide resolved
oximeter/db/src/oxql/ast/table_ops/filter.rs Outdated Show resolved Hide resolved
oximeter/db/src/oxql/point.rs Outdated Show resolved Hide resolved
oximeter/db/src/oxql/point.rs Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Outdated Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Show resolved Hide resolved
oximeter/oximeter/src/quantile.rs Show resolved Hide resolved
@zeeshanlakhani
Copy link
Collaborator Author

Last piece is the sql updates (will make those in the next commit).

Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

I took a look at the SQL migrations assuming that's the only thing that changed. Those LGTM, though there's one opportunity I think we should take for simplifying the definition of a distributed table on top of the local ones. That will really reduce the difficulty of changing the replicated table definitions.

Other than that, LGTM! Thanks for all the back and forth on this, it'll be cool to see this work merged!

oximeter/db/schema/replicated/5/up04.sql Outdated Show resolved Hide resolved
@zeeshanlakhani zeeshanlakhani enabled auto-merge (squash) June 24, 2024 20:40
@zeeshanlakhani zeeshanlakhani merged commit 8238f98 into main Jun 24, 2024
22 checks passed
@zeeshanlakhani zeeshanlakhani deleted the zl/extend-histogram branch June 24, 2024 21:12
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.

oximeter histograms could maintain more information about their inputs
2 participants