Skip to content

ore/metrics: implement Borrow for drop wrapper types#19929

Merged
teskje merged 1 commit into
MaterializeInc:mainfrom
teskje:metric-wrappers-borrow
Jun 15, 2023
Merged

ore/metrics: implement Borrow for drop wrapper types#19929
teskje merged 1 commit into
MaterializeInc:mainfrom
teskje:metric-wrappers-borrow

Conversation

@teskje

@teskje teskje commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

This PR adds Borrow implementations to the wrapper types DeleteOnDropHistogram, DeleteOnDropCounter, and DeleteOnDropGauge.

This makes it possible to write code that is generic over both the prometheus type and the wrapped type. The existing Deref impl does not work for this.

Slack thread for context.

Blocks #19964.

Motivation

  • This PR adds a feature that has not yet been specified.

Make it possible to write code that is generic over both GenericGauge and DeleteOnDropGauge.

Tips for reviewer

@petrosagg points out that perhaps we shouldn't do this because Borrow is reserved for smart pointers. I would have called the DeleteOnDrop* wrappers smart pointers, but I'm also not clear about the definition. In any case, those types already implement Deref, which is also a smart-pointer trait.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:
    • N/A

@teskje teskje marked this pull request as ready for review June 14, 2023 13:12
@teskje teskje requested a review from benesch as a code owner June 14, 2023 13:12
@teskje teskje requested review from antiguru and petrosagg June 14, 2023 13:12
@petrosagg

Copy link
Copy Markdown
Contributor

I see in the documentation of the Borrow trait a suggestion of using AsRef if the additional guarnatees of Borrow are not needed. Could we maybe add an AsRef impl to the types that we need?

If not, I think we should also add a comment in the definition of the struct warning future developers that they should not add a derive for any of Eq, Ord and Hash because that would invalidate the expectations of the Borrow impl

@teskje

teskje commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

Could we maybe add an AsRef impl to the types that we need?

I don't think that would work. The goal would be to specify a trait bound like:

G: AsRef<GenericGauge<_>>,

... and then be able to pass both DeleteOnDropGauge and GenericGauge in place of G. But there is no blanket T: AsRef<T> impl, so you'd end up in the same situation as with Deref.

Adding a comment is a good idea!

@teskje

teskje commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

Alternatively, we could just invent our own AsMetric trait if people would feel more at ease with that. Implementing Borrow is a bit scary.

But if we decide that the DeleteOnDrop* types are not smart pointers, we should probably also stop implementing Deref for them.

@teskje teskje force-pushed the metric-wrappers-borrow branch from 897a602 to e0417b3 Compare June 14, 2023 15:01
This commit adds `Borrow` implementations to the wrapper types
`DeleteOnDropHistogram`, `DeleteOnDropCounter`, and `DeleteOnDropGauge`.

This makes it possible to write code that is generic over both the
prometheus type and the wrapped type. The existing `Deref` impl does not
work for this.
@teskje teskje force-pushed the metric-wrappers-borrow branch from e0417b3 to 7d32407 Compare June 15, 2023 10:44
@teskje teskje mentioned this pull request Jun 15, 2023
5 tasks
@teskje

teskje commented Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

I've added the comments as suggested by @petrosagg. In Slack we seem to have arrived at a rough consensus that the DeleteOnDrop wrappers are something smart-pointer like ("smart handles"?), but also that the Borrow docs do not mention smart pointers anyway. So I think adding Borrow impls is fine here.

@teskje teskje merged commit dd04fae into MaterializeInc:main Jun 15, 2023
@teskje

teskje commented Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

TFTR!

@teskje teskje deleted the metric-wrappers-borrow branch June 15, 2023 13:28
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.

2 participants