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

Added IteratableGatherer for future zero alloc implementations. #927

Closed
wants to merge 1 commit into from

Conversation

bwplotka
Copy link
Member

Fixes #917

Signed-off-by: Bartlomiej Plotka [email protected]

@bwplotka bwplotka requested review from kakkoyun and beorn7 October 22, 2021 14:40
@bwplotka
Copy link
Member Author

Let's not merge, just yet -experimenting how useful it is.

@bwplotka bwplotka marked this pull request as draft October 22, 2021 14:41
// MetricFamilyIterator ...
type MetricFamilyIterator interface {
Next() bool
At() *dto.MetricFamily
Copy link
Member

Choose a reason for hiding this comment

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

This helps to protect the addition and removal of whole MetricFamilies. But it doesn't protect editing existing MetricFamilies in place. For example, if your set of metrics is entirely static, you just want to change a value now and then by directly changing only the Value protobuf message deep down within a MetricFamily, you would still create data races with this approach.

In other words, after At() is called, the iterator has no control over the usage of the returned MetricFamily anymore. You only moved the problem one level down (from the MetricFamily slice to an individual MetricFamily).

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, I plan to explicitly mention in the interface that after Next you lose control over memory space pointed by pointer. See GatherIterate method comment. Also see implementation.

I don't see any other type safe way to do it. Even my initial idea with GatherForeach(f func(*dto.MetricFamily) error) error has the same issue - someone might reuse it.

We have to rely on runtime tests and interface comments, unfortunately. But this will work. WDYT?

Copy link
Member Author

@bwplotka bwplotka Oct 23, 2021

Choose a reason for hiding this comment

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

Well - there is one type safe manner. We could tie gathering with encoding in some GatherEncode(encoder) error - but:

  • it is a little bit package spaghetti,
  • tied to promhttp use as we might have to have this non trivial error mechanism that either stops, continues or panics ...

But might be way to go

@bwplotka
Copy link
Member Author

Anyway, all those extra interfaces are doing one thing -> Gather is locked (: So let's take an easier approach for now and use promhttp Gather with lock..

@bwplotka bwplotka closed this Oct 23, 2021
@kakkoyun kakkoyun deleted the janitor2 branch August 5, 2022 18:29
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