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

WIP: Add support for prometheus with exemplars #79

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shreyassrivatsan
Copy link

Which problem is this PR solving?

The change adds support for emitting exemplars alongside metrics using the latest prometheus client.

Short description of the changes

  • Pull in latest v1.4.1 prometheus client
  • Add WithExemplar methods to counters and histograms

@shreyassrivatsan shreyassrivatsan changed the title Add support for prometheus with exemplars WIP: Add support for prometheus with exemplars Feb 28, 2020
@yurishkuro
Copy link
Member

a few notes/questions:

  • why counter/hist but not gauge?
  • what is the semantic definition of "exemplar"?
  • depending on that, perhaps passing a struct would be better, to build extensibility in the API
  • wonder if it would be feasible to keep the API intact and pass exemplar as a label (probably not great because the API is designed for static labels where the stats object is pre-created, unlike Prometheus API that takes label values dynamically.

- Pull in latest v1.4.1 prometheus client
- Add WithExemplar methods to counters and histograms

Signed-off-by: Shreyas Srivatsan <[email protected]>
Signed-off-by: Shreyas Srivatsan <[email protected]>
@shreyassrivatsan
Copy link
Author

* why counter/hist but not gauge?

prometheus/openmetrics only support exemplars on counters and histograms.

* what is the semantic definition of "exemplar"?

Exemplars stand for example values and labels emitted along side metrics. In the case of tracing it could be a traceid and/or spanid allowing one to associated a metric datapoint with a trace.

* depending on that, perhaps passing a struct would be better, to build extensibility in the API

Yes, we could certainly do that.. Instead of traceID we could pass in a map[string]string as labels and keep it in control of the user what exactly to emit.

* wonder if it would be feasible to keep the API intact and pass exemplar as a label (probably not great because the API is designed for static labels where the stats object is pre-created, unlike Prometheus API that takes label values dynamically.

Right, that will be difficult as the labels will change each time, so its better to pass it in as a parameter.

The alternate to changing the interface is adding a separate interface the same way prometheus does and have the caller check what interface it has and emit exemplars if it finds the CounterExemplar interface.

type CounterExemplar interface {
    Counter
    IncWithExemplar(...
}

@yurishkuro
Copy link
Member

Exemplars stand for example values and labels emitted along side metrics.

I realize that, but as far as the exposition format, is this purely limited to exemplars, or can it have other metadata? Is there structure to that data? I recall that OpenMetric format represented exemplars almost like comments attached to values. The point is, if the actual semantics of the attached data is larger than just exemplars, then it's the wrong name to use.

The alternate to changing the interface is adding a separate interface

That's what we'll most likely need to do, because otherwise we need bump the major version of this lib.

@shreyassrivatsan
Copy link
Author

shreyassrivatsan commented Feb 29, 2020

I realize that, but as far as the exposition format, is this purely limited to exemplars, or can it have other metadata? Is there structure to that data? I recall that OpenMetric format represented exemplars almost like comments attached to values. The point is, if the actual semantics of the attached data is larger than just exemplars, then it's the wrong name to use.

Yes, its in the comment part of the metric. But the structure is just an exemplar. # {<labels>} <value> <ts> and the only thing the prometheus client gives control of is labels. It takes the value and ts based on the metric being emitted.

The alternate to changing the interface is adding a separate interface
That's what we'll most likely need to do, because otherwise we need bump the major version of this lib.

Makes sense. I'll change this to that..

Also, I noticed one thing which will make pushing this change not easy. Importing v1.4.1 prom client pull in a go modules v2 package of https://github.com/cespare/xxhash, but dep does not have support to understand that.. There is WIP change in the dep repo (been open for over 6 months) for that - golang/dep#1963, which works but has not been merged into master.. I still need to see how things will work with glide..

@shreyassrivatsan
Copy link
Author

@yurishkuro I went ahead and changed the implementation to have separate interfaces and pass in labels rather than traceID.. The dep issue still remains though.

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