Code generation: how to avoid naming collisions

[lmolkova]

As we discovered in #1031, certain semantic conventions changes (together with default code generator behavior) result in ambiguous constant names generated in the code.

For example, when foo.bar_baz is renamed to foo.bar.baz, the code generator produces the same constant name for both -FOO_BAR_BAR , FooBarBaz, etc depending on the preferred casing.

We don't remove attributes/enum members/metrics from the semantic conventions anymore (old property is deprecated), so both constants would exist in the same version.

So, the semantic conventions together with the codegen should prevent such collisions from happening:

• The default tooling behavior should result in collision-free code.
• There could be additional policies added to semconv to prevent certain renames.

Please comment/vote on specific options listed in the comments.

Note: there could be edge cases when ambiguity is fine and tolerable or another collision resolution approach could be used. Here we want to pick a default behavior - it does not prevent someone from implementing a different approach.

The current status is documented in this comment #1118 (comment)

[lmolkova]

Option 1: foo.bar_baz is generated as FOO_BAR__BAZ, FooBar_Baz, ...

• any renames are allowed (for experimental conventions)
• _ is preserved in constant names

Cons:

• __ is ugly in snake_case and _ is ugly in PascalCase

[lmolkova]

Option 1.5: foo.bar_baz is generated as FOO__BAR_BAZ, Foo_BarBaz, ...

(same as Option 1 but a different format is used)

[lmolkova]

Option 2: foo.bar_baz is generated as FOO_BARBAZ, FooBarbaz, ...

• renames that only remove or add _ are not allowed even for experimental attributes.
• _ is removed during code generation

Cons:

• limited rename options. Partially mitigated by the fact that we didn't recall making such renames in the past.

[lmolkova]

Option 3: do nothing

• any renames are allowed (for experimental conventions)
• _ and . are not distinguishable in constant names

Cons:

• possible attribute name and schema version mismatch (instrumentation thinks it sets foo.bar_baz, but it's foo.bar.baz now).

[lmolkova]

Option 4: foo.bar_baz is generated as FOO_BAR_BAZ, FooBarBaz. When foo.bar.baz is added, the collision is detected, so the new attribute is called FOO_BAR_BAZ_NEW, FooBarBazNew.

• any renames are allowed (for experimental conventions)
• resolve collisions by giving new names

Cons:

• new attribute would have a 'bad' name and deprecated attribute would have a 'good' name forever.

The rename can be done for the old attribute, resulting in Option 3.

[marcalff]

When 2 different symbols in semantic conventions generate the same symbol on the generated code,
that is, when there is a collision, one of the name mapping has to change, and this change is by definition a breaking change: code instrumented that refers to a given name has to be adjusted to use the adjusted name instead.

To minimize the overall impact, it is preferable that only symbols that are the least frequent gets to be broken.

Any solution that changes the output for foo.bar to be different, which is the most common case, should be discarded.

So, solution 1.5 is definitively out in my opinion, it will break everything.

Assuming semantic conventions containing the _ character are relatively less represented (in all the semconv that exists), the change should be on how _ is represented, to minimize the overall impact, without touching ..

Option 2 is out, it collides on foo.bar_baz and foo.barbaz.

[Edit] Looks like I misunderstood option 2. Sounds viable if these collisions are forbidden, and it avoids causing renames with the current generated code. +1 then

Option 4 does not scale in my opinion, after several renames, and it assumes there is some "state" stored somewhere to remember if a collision ever existed in the past or not. Does not sound viable.

Option 3 does not resolve the issue, this is bound to be repeated. For new semantic conventions, a desirable goal is to be able to decide the most natural name for a given semconv, without having to look at possible collisions, that would prevent to use a good name ... especially when the colliding semconv is deprecated and will eventually be removed over time.

The only viable solution looks like option 1 or option 2.

[EDIT 2024-06-11]

See newer proposal as option 5 in this thread, which is better to 1 and 2 in my opinion.

[codeboten]

Option 2 seems the preferable approach, but just to confirm this means that any semconv generated code after this is implemented will cause a breaking change with previous versions as the variables will have been renamed, correct?

I'm specifically trying to understand what this looks like in the context of the message client id change, does option 2 mean that the client ID change will be rolled back?

[austinlparker]

Option 2: foo.bar_baz is generated as FOO_BARBAZ, FooBarbaz, ...

• renames that only remove or add _ are not allowed even for experimental attributes.
• _ is removed during code generation

Cons:

• limited rename options. Partially mitigated by the fact that we didn't recall making such renames in the past.

just to check, does that mean that messaging.client_id -> messaging.client.id gets rolled back and there's no change?

[lmolkova]

According to the Option2, there will be no collision for messaging.client_id -> messaging.client.id:

• messaging.client_id would be MESSAGING_CLIENTID or MessagingClientid
• messaging.client.id would be MESSAGING_CLIENT_ID or MessagingClientId

So I don't think we need to roll it back.

Option 2 seems the preferable approach, but just to confirm this means that any semconv generated code after this is implemented will cause a breaking change with previous versions as the variables will have been renamed, correct?

That's correct. Any option we pick (except opt3) will result in breaking changes in generated code. In most languages this is tolerable since semconv artifact is experimental.
So far we've identified JS and PHP which shipped stable semconv artifacts and JS was going to make some breaking changes anyway.
It will be a problem for PHP.

[joaopgrassi]

Another point that in this specific case: The new attribute messaging.client.id needs to produce a breaking change. We do not want to have the old const name carry the new attribute key value. That would cause instrumentations to send data in a mixed "schema" where some attributes are old and some are using the new keys.

To me, option 2 is the one that makes the most sense.

[marcalff]

Another point that in this specific case: The new attribute messaging.client.id needs to produce a breaking change.

It will not.

Existing code using MESSAGING_CLIENT_ID or MessagingClientId, corresponding to messaging.client_id, will need to be fixed to use MESSAGING_CLIENTID or MessagingClientid instead, once code generation is fixed.