fix: correct UCUM units for TiBy and kilobytes in unitMap

[Naman-B-Parlecha]

Fixes two incorrect UCUM unit mappings as mentioned in #65

1. TiBy should map to tebibytes and not tibibytes
2. kilobytes should map to kBy and not KBy

[Naman-B-Parlecha]

PTAL @ArthurSens @dashpole
Thanks!

[aknuds1]

@Naman-B-Parlecha @dashpole Some questions:

1. Won't we need to update the tebibytes spelling simultaneously also in opentelemetry-collector-contrib?
2. Won't renaming "tibibytes" metrics units to "tebibytes" be a backwards compatible change in Prometheus, that might break things for users?
3. If we stop supporting "KBy" as an OTLP unit, won't that silently break support for producers sending the "KBy" spelling?
4. Won't we need to update the "KBy" spelling simultaneously also in opentelemetry-collector-contrib?

[dashpole]

Sure. I'm OK keeping KBy for backwards-compatibility. We can also coordinate the update to the collector.

FYI @Naman-B-Parlecha that is here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/138fbe1878d1b8c8bd31e3c942ec0d34e08c0c87/pkg/translator/prometheus/unit_to_ucum.go#L24

[aknuds1]

Thanks @dashpole - WDYT about my concern that renaming "tibibytes" metrics units to "tebibytes" will be a backwards compatible change in Prometheus, potentially breaking things for users though?

[Naman-B-Parlecha]

Thanks @dashpole - WDYT about my concern that renaming "tibibytes" metrics units to "tebibytes" will be a backwards compatible change in Prometheus, potentially breaking things for users though?

Worth adding a warning log where this conversion happens in prometheus/prometheus for user visibility?

@dashpole @aknuds1

[aknuds1]

I'm wondering whether this backwards compatible change should be possible to disable, e.g. for configurability from Grafana Mimir. As for Prometheus itself, maybe the old behaviour should be enabled until v4? As backwards incompatible changes shouldn't be made until the next major version (of Prometheus). @dashpole @Naman-B-Parlecha.

WDYT @ArthurSens @krajorama @jesusvazquez @ywwg?

[ArthurSens]

Hmmmm, while I understand the potential negative effect, I'm not sure the units being changed are popular enough for us to be THIS careful.

I can see kylobytes being used somehow (and we're keeping the backward compatible behavior), but Tebibytes? 😅

I don't think this is used by any Prometheus exporter, and it's not part of any semantic conventions in OTel either. The only way this is used is when someone creates a custom metric and intentionally measures things in Terabytes. There are very few companies in the world that measure things on this scale.

I'd be super happy if we could just do a patch release for this, but if you're feeling very strongly opinionated about this, we could do a 2.0. What do you think?

[aknuds1]

@ArthurSens I'm not suggesting a major bump for otlptranslator, but potential configurability. I'm further suggesting that we might want to disable this new behaviour in Prometheus until v4, for backwards compatibility. It's a question from my side though, and I'd like feedback from other maintainers.

As for Grafana Mimir, the configurability could be good to have to protect users from potential metric renames.

[dashpole]

I'm not that worried about this this breaking users. I think this is a case where it is clearly a bug fix, and unlikely to have a significant negative impact. If upstream projects want configurability, it should be pretty easy for them to add special handling for TiBy prior to calling into this library to preserve existing behavior.

I would probably advocate for using a feature gate to allow users to temporarily revert back to the old name (and provide feedback, if necessary) for a period of time.

[aknuds1]

@dashpole That's what I'm suggesting too. We already have precedence for such configuration parameters in otlptranslator, see:

• PreserveMultipleUnderscores
• UnderscoreLabelSanitization

@Naman-B-Parlecha The above two are examples of configuration parameters controlling backwards compatible translation behaviour.

[dashpole]

Sure. Lets do that then

[Naman-B-Parlecha]

Hey @dashpole @aknuds1 @ArthurSens!
Apologies for the delay, have added the configurable flag for updated metric namer

one concern i have is naming currently added UpdatedMetricMapping, if u guys any better convention happy to use the suggestion!!

[Naman-B-Parlecha]

@dashpole PTAL added two new strategies UnderscoreEscapingWithUpdatedSuffixes and NoUTF8EscapingWithUpdatedSuffixes

[dashpole]

We shouldn't need a new strategy, right? Just booleans on the Namer structs (they should probably be "legacyUnitMapping" or something like that so that the default value of false uses the new behavior).