otlp: Migrating translator packages #16089
Conversation
storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go
Outdated
Show resolved
Hide resolved
go.mod
Outdated
| @@ -5,6 +5,7 @@ go 1.22.7 | |||
| toolchain go1.23.4 | |||
|
|
|||
| require ( | |||
| github.com/ArthurSens/otlp-prometheus-translator v0.0.0-20250228115610-6db8fa672349 | |||
There was a problem hiding this comment.
Thanks for splitting this into new module! Would be super useful for downstream callers that support otlp ingestion like Thanos.
I wonder if a good place for this module might be in https://github.com/prometheus/client_golang/tree/main/exp? As this is zero dep already, will probably be treated as experimental for some time and will have several downstream callers. Wdyt? cc: @bwplotka
There was a problem hiding this comment.
I think it sounds very out of scope for client golang🤔, but super happy to move elsewhere in the Prometheus org
There was a problem hiding this comment.
Sorry, I didn't fully clarify my thinking there. client_golang scope to me is "instrumentation lib + clients/utilities to interact with Prometheus and its APIs". otlp (like remote write) is an accepted way of interacting with Prometheus, so code for otlp translation, can probably live there as a submodule (near some of the remote write API handling code that is in exp). 🙂
However, not blocking anything, prometheus/common would be good too.
There was a problem hiding this comment.
Yea, I would recommend separate repo for now, so it can have more external maintainers e.g. in prom org, no harm in that vs it might overload client_golang.
storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go
Outdated
Show resolved
Hide resolved
bbc7321 to
0d0db32
Compare
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
0d0db32 to
a767f14
Compare
|
I'm testing whether this will work with Grafana Mimir. |
| @@ -762,7 +762,7 @@ func TestPrometheusConverter_addExponentialHistogramDataPoints(t *testing.T) { | |||
| Settings{ | |||
| ExportCreatedMetric: true, | |||
| }, | |||
| prometheustranslator.BuildCompliantMetricName(metric, "", true), | |||
| translator.BuildCompliantMetricName(metric.Name(), metric.Unit(), getTranslatorMetricType(metric), true), | |||
There was a problem hiding this comment.
Why does the translator operate with its own metric types, instead of using metric.Type? That looks like a bad duplication to me. Also, why can't translator.BuildCompliantMetricName just take metric as before? The new API looks like a change for the worse for me.
There was a problem hiding this comment.
Basically, I'd like rationale for the changes made in common versus the corresponding code in storage/remote/otlptranslator/prometheus. They don't look like an improvement, but I'm open to the possibility that I'm missing something.
There was a problem hiding this comment.
We wanted to be dependency free so it's easy to import anywhere.
Making the library dependent on Otel, and then Otel dependent on the library would be super hard to work with.
Is the signature change that bad in your opinion? We're not passing a giant object anymore, so it might even have better performance...? (I haven't done benchmarks about this specifically)
There was a problem hiding this comment.
We wanted to be dependency free so it's easy to import anywhere.
Making the library dependent on Otel, and then Otel dependent on the library would be super hard to work with.
I will try myself, to see what the dependency problem looks like. Thanks for the explanation.
There was a problem hiding this comment.
Not sure if I explained it well, the problem is circular dependency
There was a problem hiding this comment.
Yeah I understood you meant a circular dependency. I just have to try for myself, to see whether it can be avoided.
There was a problem hiding this comment.
@ArthurSens See my attempt at using the OTel SDK instead. There's no circular dependency when combining prometheus/common and prometheus/prometheus. Does it somehow happen when combining with OTel? If so, please provide a demonstration.
There was a problem hiding this comment.
From our discussion in Slack, it was established that depending on OTel SDK metric types, as in my prototype, most likely doesn't cause any circular dependencies after all.
There was a problem hiding this comment.
Truly sorry, but I can't review this PR in its current state because it's too painful due to how much changes in prometheus/common/otlptranslator versus the current code base.
To enable me to review the PR, please do the following:
- Please revert the
prometheus/common/otlptranslatorpackage to using the OTel SDK (go.opentelemetry.io/collector/pdata) as in my branch (as no cyclical dependencies can be established as was originally claimed). - Please revert the
prometheus/common/otlptranslatorpackage code to be as similar as possible to the code being moved from the prometheus/prometheus repo. Diffing is currently impossible, due to code changing around so much.
After an initial PR with the sole goal of moving code from prometheus/prometheus to prometheus/common, we can think about other improvements. Let's not do everything at once, since it puts too much burden on the reviewer and I don't feel safe about the changes. As a rule of thumb, PRs should in any case be single topic.
|
superseeded by #16240 |
As discussed with multiple peers in the last couple of quarters(yes, quarters 😅), @ywwg and I started experiments to avoid duplicated code between Prometheus and OpenTelemetry-Collector for translating OTel to Prometheus metric and label names.
The experiments led to https://github.com/ArthurSens/otlp-prometheus-translator, but I'm very happy to move it somewhere else, like prometheus/common, or into a separate repository in the Prometheus org, where we can make other folks maintainers.
EDIT: Package moved to https://github.com/prometheus/common/tree/main/otlptranslator
Motivation for the new module
Goals
Opening as a draft to continue discussions! I think we want to ask/answer questions before committing