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

Extract HDR histogram implementation into a shared package #4611

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

joanlopez
Copy link
Contributor

@joanlopez joanlopez commented Mar 6, 2025

What?

It extracts the HDR histogram implementation from the expv2 Cloud output package, into an internal but shared package, so it can be reused for our metrics.TrendSink implementation and/or (at the very least) for the Summary output.

Why?

Because as discussed and suggested here it's likely the best way to move forward. Plus, now that we're about to release a new major, it's likely a good moment to add this kind of breaking change (behavioral, not programmatic API).

Related PR(s)/Issue(s)

Related with #763

Notes for the reviewer(s)

To facilitate the review, I'd suggest to do a clipboard diff between the contents that were on expv2/hdr.go vs the new histogram/hdr.go. But to summarize, it's mostly about making the type and constructor public, and leaving the protobuf specific bits in the Cloud output package, because it feels like it's specific to that.

Also, note that I decided to create this histogram package inside a ds directory, standing for "data structures", but I'd be happy to either rename ds into something more explicit, and/or move histogram into a first level package inside internal. As it is internal, it should be fine to start this way and move into a ds package tomorrow if we ever add more data structures. Just tell me what you prefer.

@joanlopez joanlopez added this to the v1.0.0-rc1 milestone Mar 6, 2025
@joanlopez joanlopez self-assigned this Mar 6, 2025
@joanlopez joanlopez requested a review from a team as a code owner March 6, 2025 17:52
@joanlopez joanlopez force-pushed the extract-common-hdr branch 2 times, most recently from 7032657 to 303d9df Compare March 6, 2025 18:05
@joanlopez joanlopez force-pushed the extract-common-hdr branch from 303d9df to 9943cd4 Compare March 6, 2025 18:08
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🚀 🎉

I think it makes sense to have a data structure specific package, for reusability, and to centralize a lot of the logic we might reuse over time. My only complaint is that I find the ds name implicit and if I were looking for histograms I wouldn't think "oh yeah, it's gonna be in that ds folder" :D, although I could live with it (non-blocking).

A more specific/explicit name I can think of for the package would be container in that sense it expresses that it contains data structures dedicated to containing data. But I could see a histogram package at the top level of internal.

@mstoykov
Copy link
Contributor

mstoykov commented Mar 7, 2025

I think it is fine to have it in a separate hdr/histogram package for now and later if we decide to have datastructure package to ... have it named that than ds.

But IMO we can do that later as well

@joanlopez
Copy link
Contributor Author

Gonna merge it as-is, to move forward. As it is an internal package, we have freedom to adjust as needed later.

@joanlopez joanlopez merged commit ed02613 into master Mar 10, 2025
28 checks passed
@joanlopez joanlopez deleted the extract-common-hdr branch March 10, 2025 10:09
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.

3 participants