Add constants to facilitate migration of Collector's prometheusexporter

[ArthurSens]

I'm opening this PR to start a quick discussion.

While working on migrating the translation package for the Collector's Prometheus exporter (without adding any new functionality to the exporter), I noticed that a few constants from the old library are used.

I've opened this PR just to showcase what constants we would need to add to our library to enable a transparent migration.

If we'd like to add them, that's a different story. If we don't want them, where would be the best place for those libraries to live?

[ArthurSens]

See open-telemetry/opentelemetry-collector-contrib#39753 for an example of how the migration would look if we adopt those constants.

[aknuds1]

Can you explain where these constants would be used? I personally don't think it's a good idea considering adding them to otlptranslator without a concrete use case. It's probably a better idea if you start in the other end, and do whatever refactoring you have in mind first, before suggesting moving constants to this shared library.

[ywwg]

I feel like these constants are a part of the standard translation from Otel to Prometheus so it does belong here. +1

[ArthurSens]

Yes, all constants in this PR are documented at OTel's Prometheus and OpenMetrics compatibility page..

But the question is, is our vision for this library to implement all translations between OTel and Prometheus, or just metric and label names?

[aknuds1]

But the question is, is our vision for this library to implement all translations between OTel and Prometheus, or just metric and label names?

I still don't think we should be adding constants without a defined use case first. Otherwise, future maintainers will just be asking themselves what the constants are doing there.

[ArthurSens]

It is being used by most (I haven't checked all) components from Prometheus components in the collector, a few examples:

• https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/prometheusreceiver/internal/metricfamily.go#L474
• https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/prometheusexporter/collector.go#L161
• https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/prometheusremotewrite/otlp_to_openmetrics_metadata.go#L17

There are too many constants to look for every usage, but they are generally used to comply with the spec

[aknuds1]

@ArthurSens Do those cases match the defined purpose of this library ("Library to translate string from OTLP format to Prometheus format")? Never mind, I remembered your question about whether to expand the scope of the library. However, the library's name is otlptranslator, so wouldn't we also have to change the name?

[aknuds1]

@ArthurSens Could you open an opentelemetry-collector-contrib draft PR demonstrating the constant usage you have in mind? I would much prefer if we had a practical example to evaluate what you are suggesting. E.g., why can't the constants live in opentelemetry-collector-contrib instead?

[ArthurSens]

@ArthurSens Do those cases match the defined purpose of this library ("Library to translate string from OTLP format to Prometheus format")? Never mind, I remembered your question about whether to expand the scope of the library. However, the library's name is otlptranslator, so wouldn't we also have to change the name?

Yeah, that's my doubt as well 😅... Maybe increasing the scope is too ambitious. I think the name otlptranslator still works, the spec is a guide about transforming OTLP into Prometheus formats... otlptranslator sounds appropriate.

@ArthurSens Could you open an opentelemetry-collector-contrib draft PR demonstrating the constant usage you have in mind? I would much prefer if we had a practical example to evaluate what you are suggesting. E.g., why can't the constants live in opentelemetry-collector-contrib instead?

Yes, I'll open another PR without moving constants over here. But that's a lot of work, it might take a while

[aknuds1]

Maybe increasing the scope is too ambitious

I don't think you should consider ambition a valid reason for adding code to this library. Code should only be added if there's a concrete need for it, otherwise we just increase our workload/cognitive load.

Yes, I'll open another PR without moving constants over here. But that's a lot of work, it might take a while

It certainly can't be more work than using constants in a separate library.

[aknuds1]

I'll open another PR without moving constants over here

@ArthurSens It's entirely up to you whether you want to open a PR, but I definitely think a demonstration PR should be opened before suggesting to add constants to this library instead of directly in opentelemetry-collector-contrib.

[ArthurSens]

Here is a PR demonstrating what we'd need to do if we don't migrate the constants here.

We have 3 components in the collector duplicating code

[aknuds1]

open-telemetry/opentelemetry-collector-contrib#39827 demonstrating what we'd need to do if we don't migrate the constants here.

That PR doesn't explain why open-telemetry/opentelemetry-collector-contrib#39753 is supposed to require moving constants to otlptranslator though (it just implies the need). Maybe you can instead explain in the context of open-telemetry/opentelemetry-collector-contrib#39753, why it would depend on the constants being in otlptranslator?

[ArthurSens]

That PR doesn't explain why open-telemetry/opentelemetry-collector-contrib#39753 is supposed to require moving constants to otlptranslator though (it just implies the need). Maybe you can instead explain in the context of open-telemetry/opentelemetry-collector-contrib#39753, why it would depend on the constants being in otlptranslator?

There's no requirements here, it's just an idea to get things in a good state. If we don't move the constants to this library, do you have any other suggestions?

[aknuds1]

@ArthurSens I'm going to reach out to you on Slack, because I need concrete information from you on why these constants would be necessary in otlptranslator. It might be better if we discuss synchronously.

[ywwg]

target_info, trace_id, span_id, otel_scope_name, and otel_scope_version all defined in https://github.com/prometheus/prometheus/blob/main/storage/remote/otlptranslator/prometheusremotewrite/helper.go

https://github.com/prometheus/prometheus/blob/main/storage/remote/otlptranslator/prometheusremotewrite/helper.go#L56-L59
https://github.com/prometheus/prometheus/blob/main/storage/remote/otlptranslator/prometheusremotewrite/helper.go#L180-L182

I can't find any reference to prometheus.type or otel_scope_info in the prom source code

[aknuds1]

Thanks @ywwg - can we update the PR to just include those constants for which there exists a sharing use case?

[ywwg]

done

[aknuds1]

LGTM, thanks.