Semconv codegen should produce different constant names if attribute is renamed _ -> ``

[lmolkova]

Code generator generates the same attribute name constant for attribute.name and attribute_name. As a result, if attribute is renamed (_ is changed to . or vice versa), the value of attribute changes, but the name stays the same.

It's ambiguous - see open-telemetry/semantic-conventions#1031 and open-telemetry/semantic-conventions#1118 for the discussion.

Based on open-telemetry/semantic-conventions#1118, the semconv recommendation would be to:

• Remove _ when generating any constants in the code (fields, methods, class names, etc)
• Semconv will prohibit attribute renames that are limited to adding/removing _

This PR demonstrates the result of such naming policy change.

[trask]

do we want to apply this change here (and other values)?

[trask]

😢

[lmolkova]

I agree, the only way it can be tolerated is if we avoided multi_word_names as much as possible.

[trask]

from an API user perspective, it looks like a typo that we forgot some underscore(s) in the constant name 😢

[trask]

what do you think about alternative proposal:

• first one gets the good constant name: MESSAGING_CLIENT_ID = "messaging.client_id"
• next one (after rename) gets a "bad" constant name: MESSAGING_CLIENT_ID2 = "messaging.client.id"
• when stabilizing, we are moving from MessagingIncubatingAttributes to MessagingAttributes and so the stable one can get the good constant name at that time: MESSAGING_CLIENT_ID = "messaging.client.id"
• if anything is renamed after stability, we'd need to do a major version bump anyways (although users of the previously stable constant name would silently get the new constant value without any compilation warnings, which is not great, but really hoping this kind of rename after marking something stable is a rarity 😬)

[crossoverJie]

I prefer this plan.

[lmolkova]

we could also just tolerate it (for experimental ones):

• generate both as MESSAGING_CLIENT_ID, override the value (non-deprecated wins) - it's tolerable for experimental attributes
• as you mentioned, we'd probably never do a rename like this for a stable attribute - the value would be too low to justify the cost.

[lmolkova]

I'm not excited about MESSAGING_CLIENT_ID2 since it would either need to be stabilized with this const name or users would need to update IncubatingAttributes.MESSAGING_CLIENT_ID2 -> Attributes.MESSAGING_CLIENT_ID

[lmolkova]

The problem still remains - we don't really know the order in which attributes were added and which one should become MESSAGING_CLIENT_ID2. We can assume that deprecated should get MESSAGING_CLIENT_ID, but what if we deprecated both?

[trask]

What would we put into stable artifact when it's stabilized? MESSAGING_CLIENT_ID?

👍

[lmolkova]

we can do it with some manual interventions.
Given that collisions are rare, it should be ok.

Trying it out here 6390847

[trask]

I don't mind the approach 3.5 where we accept breaking changes in the incubating artifact, but wondering if this solution could work better for folks who are more apprehensive about the breaking changes (and the strategy could also be used to mitigate the even rarer attribute type changes on experimental attributes)

[lmolkova]

#76 shows manual conflict resolution:

• it allows to change constant names for experimental attributes. E.g. say that messaging.client.id constant names should be generated as if it was messaging.client.id_new (preserving the const value)
• or drop an attribute based on the name