Skip to content

Conversation

@bwplotka
Copy link
Member

Unfortunately optionality is important as not every e.g. ObserverVec implements exemplar (e.g. summary).

For counters this is not that needed - but changed for consistency and readability.

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Besides coding style nit.

And one more thought: Perhaps add do the doc comment of WithExemplarFromContext that using that option with a observer implementation that doesn't support exemplars will panic.

@beorn7
Copy link
Member

beorn7 commented May 2, 2023

Could we expedite this and cut a 1.15.1 with it soon?

See prometheus/pushgateway#550 as an example for the fallout.

@bwplotka
Copy link
Member Author

bwplotka commented May 2, 2023

Yup, will do, thanks for ping!

@bwplotka bwplotka changed the base branch from main to release-1.15 May 3, 2023 08:40
@bwplotka
Copy link
Member Author

bwplotka commented May 3, 2023

I decided to switch direction and revert 3ce88f3 commit & add quick commentary.

Might be better to avoid panics even in cost of confusion for people "why their exemplars don't work"

@bwplotka
Copy link
Member Author

bwplotka commented May 3, 2023

Sorry @dimonl for bad suggestion on this one 🙈

@bwplotka
Copy link
Member Author

bwplotka commented May 3, 2023

Merging, since it's a revert and tests are passing

@bwplotka bwplotka merged commit 2eaffbd into release-1.15 May 3, 2023
@bwplotka bwplotka deleted the fixpro branch May 3, 2023 08:50
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

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.

5 participants