-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add exemplar support #65
Conversation
a6ef2a3
to
9be037a
Compare
ff71d87
to
97476e9
Compare
a6dd8c1
to
83f24b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! 👏🏼 👏🏼
Left some minor comments
One of the things I was thinking is, since this is going into a 2.0 milestone, we should embed exemplar support into every metric type that supports it, and easily provide adding an implicit noop that anyone can import. It would greatly simplify the codebase.
object LabelName extends internal.Refined.StringRegexRefinement[LabelName] with ExemplarLabelNameFromStringLiteral { | ||
protected val regex: Pattern = "^[a-zA-Z_][a-zA-Z_0-9]*$".r.pattern | ||
|
||
implicit val ordering: Ordering[LabelName] = catsInstances.toOrdering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: You should be able to remove it since cats' Order
already provides this as an implicit conversion.
This comment follows the conventionalcomments.org standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep it does, but we're going the other way as SortedMap
needs Ordering
|
||
implicit val ordering: Ordering[LabelName] = catsInstances.toOrdering | ||
|
||
override protected def make(a: String): LabelName = new LabelName(a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why this method?
This comment follows the conventionalcomments.org standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comes from our internal refinement code
override protected def make(a: SortedMap[LabelName, String]): Labels = new Labels(a) | ||
|
||
override protected def test(a: SortedMap[LabelName, String]): Boolean = | ||
a.nonEmpty && a.map { case (k, v) => s"${k.value}$v".length }.sum <= 128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Very clever!
This comment follows the conventionalcomments.org standard
Exemplars allow us to attach transient labels like trace IDs to metrics
e956dad
to
f858c37
Compare
We're breaking bincompat
f858c37
to
68dce79
Compare
Co-authored-by: Alejandro Hernández <info@alejandrohdezma.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exemplars allow us to attach transient labels like trace IDs to metrics
Note that some of this is still WIP, especially
Timer
andOutcomeRecorder
. We'll be doing some more refactoring before coming back for those