-
Notifications
You must be signed in to change notification settings - Fork 0
Initial example of generating Prometheus SDK metrics from Otel semconv with the weaver tool. #1
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
Conversation
"of a very important metric that everyone is using.", | ||
}, []string{"integer", "category", "fraction"}) | ||
customStableMetric.WithLabelValues("101", "AType", "1.22314").Inc() | ||
switch *metricDefinition { |
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.
Hope the intention is clear. This binary will help us showcase the case of e.g. same type of container runs with different versions where metric name changed between e.g.
my-app [email protected]
my-app [email protected]
This looks great! I need to process your feedback and add issues for us to work through. Thought I'd answer an "easy" one though.
This is how otel semconv defined units: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/metrics.md#instrument-units It comes from the UCCM standard for units. |
This could be the complexity of spans/events leaking through to metrics. On metrics we tend to default everything to required or "opt_in" (i.e. user most provide a feature flag for the label). On Spans or Events(logs) it's more likely the set of labels has recommended or not-always available things. Since weaver is designed for all of them, and attributes (labels) are shared between them all, you see that hit metric definitions. cc @lquerel @lmolkova for thoughts on simplifying metric definitions or defaulting to required. |
See "curly braces" in https://ucum.org/ucum. Curly braces are an "annotation", and are equivalent to a unit of |
Thanks for the feedback! really appreciate it! A few questions to clarify
Do you mean for something like
?
you mean that otel attributes are fully qualified and it's not the case for prometheus?
back to @jsuereth comment - check out http request duration. We document (for instrumentations and consumers) that For codegen purposes the difference they make is that:
I'm curious how you see the world without them :) |
I need to go through all of your feedback. Your insights are extremely valuable and will most certainly lead to many improvements. FYI, there is an example of a type-safe API for Rust in the Weaver repository, which I believe aligns with the direction you want to take. I had started exploring a Go version of this type-safe approach. As soon as I get my hands on it, I’ll share it with you. The Rust experiment is visible here -> https://github.com/open-telemetry/weaver/tree/main/crates/weaver_codegen_test Thank you! |
@bwplotka @vesari I retrieved a very old version of Weaver that predates the migration to the OTEL repo and contains a version of my experiment with a type-safe API for Go. The generated API followed the OTEL model, but the general approach is likely applicable to Prometheus as well. https://github.com/f5/otel-weaver/tree/main/templates/go/otel The
The wrapper structs are used to create a distinct custom type for each attribute. So, even though both http_method and http_url are strings for example, they are treated as two separate types at the API level. The wrapper struct acts as a decorator around the string type, which should be zero-cost once compiled. Interface markers were used to ensure that attributes could only be used in the appropriate context. Let me know if you need more details. I could also potentially port this experiment to the latest version of Weaver. |
For the documentation improvements, I created the following issue to track progress. |
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.
Just left a question about a future TODO. Wonderful work, thank you!
return promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ | ||
Name: "my_app_custom_elements_total", | ||
Help: "Custom counter metric for my app counting important elements. It serves as an example of a very important metric that everyone is using.", | ||
// Unit: "elements" // TODO(bwplotka): Add Unit as one of the supported options. |
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.
Not strictly related to this PR: have you already thought how we should add Unit support?
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.
Yup, we add a field and put that in the UNIT field I presume (but it's not mandatory). So some part of your initial PR if you want to get that done 💪🏽
…v with the weaver tool. Signed-off-by: bwplotka <[email protected]>
Signed-off-by: bwplotka <[email protected]>
Amazing, thanks everybody for feedback! The unit definition now makes sense, noted 👍🏽 Thanks for creating issue with all the feedback around docs @lquerel 💪🏽 Answering your @lmolkova questions: Namespacing:
Yea, that's one way of trying to get to the short, yet unique name for the group of attributes in a single "Go" package (or group of constructs in any other language). For
Instead of
However, that id format is obviously not consistent e.g. if we take http.client.request.duration. This is a bit similar to protobuf where you have a package and then it's not like every proto Message has each message prefixed with a package name, but something that is unique within a package. ANYWAY, not a blocker. This might be a little bit inconvenient for Go, in Java long names might be desired etc. Trying to optimize for the dev experience using that generated code.
Correct, and I understand why they are so (needed for more reliable correlation), but it's just not helping in my goal for the shortest unique string. (Plus it might be cumbersome to use in any metric query language, but that's a separate issue 🙈 ). As per requirement levels:
Thanks for explaining, those advanced cases makes sense generally. However, common Prometheus SDKs paths usually makes it simpler and you always have static number of dimensions/labels per binary and it makes sense in the local binary scope. You are right that some attributes/label optionality could make sense here 🤔 I thought we could use registry versioning to tell all metrics are required there and have separate one for experimental ones... Anyway, I removed all requirement fields from my semconv metrics and assume all is required to show simple usage. |
@lquerel noted the Go generation for Otel you did in https://github.com/f5/otel-weaver/tree/main/templates/go/otel You follow the typed methods which what I have here too https://github.com/f5/otel-weaver/blob/main/templates/go/otel/meter/metric.tera#L54 This is great but my thoughts are bit further, what we can optimize here given the underlying Prometheus client_golang SDK internals. Eventually all label values are string, so it's bit expensive to translate to string on every metric reference. Anyway, lot's of cool fun options with the generated code here! 💪🏽 |
@bwplotka I completely agree with you on this. The goal of this proof of concept was to add a type safety layer on top of the generic SDK client. The medium-term objective is to create optimized SDK clients for a specific registry (per service/app), effectively removing all existing abstraction layers. This would significantly reduce the overhead related to instrumentation. It’s just that, at the time, I didn’t have the time to do it. If label values must all be strings for Prometheus, that’s precisely the kind of thing that can be specifically adapted/optimized in the generated code. |
First iteration of the use of weaver to define and generate Prometheus SDK metrics in Go.
It was surprisingly fast and simple to developo, kudos to Otel community for this 💪🏽 Took mi ~2.5h to craft this. The weaver tool is ultra fast, with super helpful diagnostics and errors, super useful (I used not even prod optimized version).
Generally this forge doc was a good place to start for me. Some initial list of things to potentially improve/contribute to weaver/semconv:
kebab_case
function comes from and what are other functions like this. Turns out those are defined in weaver, ideally we link to this file or so.groups.
(the fact that you havectx
variable witheach
setting)..
for metric identification. For namespace (e.g. for unique Go package name) I would take something that will give memy-app
from somewhere. There's a little bit of chaos here, not sure what would help - maybe explicitnamespace
andshort_name
for metrics and labels? The prefixed attribute names in the official Otel semconv does not help here 🙈 Another solution would be to have some weaver j2 helper functions for that.unit
is inside curly brackets e.g. "{goroutine}" and some are strings like “By”.attribute
is simplylabel
andinstrument
is a metric type. Definitely ok for the first pass, but perhaps there's some room for simplified schema that generate Otel semconv schema 🙈Next steps for our demo/work @vesari