`messaging.client_id` -> `messaging.client.id` rename causes issues with code generation

[dyladan]

trafficstars

Area(s)

area:messaging

What happened?

Last week in https://github.com/open-telemetry/semantic-conventions/pull/948 messaging.client_id was renamed to messaging.client.id. In the JS code generator, we use {{ attribute.fqn | to_const_name }} to generate variable names. This results in conflicting constants with the same name MESSAGING_CLIENT_ID. I'm not sure anything can be done about this, but wanted to raise it to the semconv group as this is the first time I've seen such a conflict.

Semantic convention version

main

Additional context

We're currently updating our semconv package. We continue to export deprecated attributes in order to make changes to the package non-breaking. The conflicting names get in the way of the code generator. I don't want to special-case the name for a single attribute if I can avoid it, and the "good" name is currently squatted on by the old deprecated attribute.

[dyladan]

Looks like this change has not yet been released so there is still time to do something to avoid the collision. I would greatly appreciate it if it could be taken into consideration with the new name.

[trask]

it looks like this has happened before:

• messaging.message_id -> messaging.message.id
• messaging.kafka.message_key -> messaging.kafka.message.key
• messaging.rocketmq.message_type -> messaging.rocketmq.message.type
• messaging.rocketmq.message_tag -> messaging.rocketmq.message.tag
• messaging.rocketmq.message_keys -> messaging.rocketmq.message.keys

but hasn't cause a codegen problem yet because the deprecated attributes were just completely dropped from the yaml files in these cases

[dyladan]

I'd be interested to see if this affects other language code generators

[trask]

Yeah I think it's likely cc @jack-berg

[lquerel]

It's interesting to note that from the perspective of the user of the generated semconvs, this scenario is ideal because it does not require changing references to these constants. Wouldn't one possible approach be to consider that in the case of such a conflict, only the non-experimental version is retained?

[trask]

Wouldn't one possible approach be to consider that in the case of such a conflict, only the non-experimental version is retained?

I agree this may be the best option

It's interesting to note that from the perspective of the user of the generated semconvs, this scenario is ideal because it does not require changing references to these constants.

the non-ideal part is that it will automatically change (some of) the emitted telemetry to a newer schema version while the instrumentation is still emitting an older schema version url

[MrAlias]

This looks to be affecting the Go code generation: https://github.com/open-telemetry/opentelemetry-go/actions/runs/9180409074/job/25244759934?pr=5394

[jsuereth]

Previously, when renaming of this variety we DROPPED the old attribute. Now we do not.

I see a few paths forward here:

1. We update uniqueness rules in semconv to be stricter - i.e. we know common codegen does NOT distinguish between . or _ so we update a name conflict policy to catch this ahead of time. This will avoid the conflict in the future, but still requires attention to the issue today.
2. We update codegen to use the more recent value (Stable overrides others, Experimental overrides Deprecated, etc.) This has issues to it as well.
3. We remove the old attribute. Effectively, we consider a rename of . to _ or vice-versa a non-breaking change. I don't think this is a valid option, just listing it so folks know we thought of it.
4. We update codegen constant naming to distinguish . and _ in some fashion, and require languages to make this distinction. This may be a viable LONG term direction if we make new codegen artifacts for semconv across languages, but this does not fix any short term issues with libraries that want to make stability guarantees.
5. We back out the change that broke codegen and re-introduce it once we have a plan for stable codegen.

In any case, I'll take the blame for not having more discussion of this issue prior to release. It's my opinion that the correct short (and longer term fix) will be on the codegen side. I shouldn't have forced that decision though.

[brettmc]

Also affecting PHP codegen, for the same reasons as others: {{ attribute.fqn | to_const_name }} ends up with the same const name. For the time being, I can manually fix it by removing the deprecated one.

[AlexanderWert]

In SemConv we differentiate between . and _, in code generation usually not. So I'd say conceptually it's an issue with code gen that needs a cleaner long-term solution. IMHO, we should aim for Option 4 long term:

4. We update codegen constant naming to distinguish . and _ in some fashion, and require languages to make this distinction. This may be a viable LONG term direction if we make new codegen artifacts for semconv across languages, but this does not fix any short term issues with libraries that want to make stability guarantees.

I don't think Option 1 is a good solution for SemConv, as . and _ have clear, separate semantics (i.e. namespace separation vs. attribute name). By treating those as "the same thing" regarding uniqueness we would mix these semantics and make it even more confusiong.

So, for short term that would leave us (IMHO) with options 2. or 5.

[trask]

maybe:

• messaging.client_id -> MESSAGING_CLIENTID
• messaging.client.id -> MESSAGING_CLIENT_ID

it's not perfect, and we could still end up back here if there's a rename from messaging.clientid to messaging.client_id (or vice-versa), but it's probably a lot less likely than the more common renames that we have been doing:

• messaging.message_id -> messaging.message.id
• messaging.kafka.message_key -> messaging.kafka.message.key
• messaging.rocketmq.message_type -> messaging.rocketmq.message.type
• messaging.rocketmq.message_tag -> messaging.rocketmq.message.tag
• messaging.rocketmq.message_keys -> messaging.rocketmq.message.keys

[jsuereth]

We had some discussion on this in the semconv tooling group. I think there's a few options for how to rename keys, particularly:

• You can migrate . to _ and _ to __.
• Go, today, erases to camelcase. We could think about preserving _ and having . turn into camel-case otherwise.

No matter the path forward, we're going to pull together a quick statistic of how many _ we have in semconv to better understand the potential issues/impact going forward for not disambiguating . and _ in codegen.

[MadVikingGod]

Follow-up from the SC tooling meeting: It was asked how many attributes currently have an underscore to measure the impact of how the above changes might effect generated code:

$ cd model/registry
$ yq '.groups[].attributes[].id' *.yaml -r  | grep '_' | sort | uniq | wc -l
121

In addition to a number of prefix's with _

[MadVikingGod]

I would also consider there are a number of attributes that might become a namespace of another attribute if we convert _ to .. For example we have attributes:

"container.command"
"container.command_args"
"container.command_line"
"http.request.method"
"http.request.method_original"

A blanket rewriting could make container.command both an attribute and a languages' namespace for container.command.args, and the same for http.request.method to http.request.method_original.

I think we might want to make a more flexible template, maybe one that lets the users of codegen specify how the normalization works best for their language.

[dyladan]

Wouldn't one possible approach be to consider that in the case of such a conflict, only the non-experimental version is retained?

I agree this may be the best option

That sounds nice but we're getting 2 generated constants that conflict with each other and cause compilation issues. We would have to then post-process the generated code to remove conflicts, which seems clunky at best. If the generator could handle these collisions on its own maybe it would be ok

It's interesting to note that from the perspective of the user of the generated semconvs, this scenario is ideal because it does not require changing references to these constants.

the non-ideal part is that it will automatically change (some of) the emitted telemetry to a newer schema version while the instrumentation is still emitting an older schema version url

I agree we don't want the telemetry to change out from under the user. Seems likely to result in telemetry where the telemetry doesn't match what its schema url claims.

We had some discussion on this in the semconv tooling group. I think there's a few options for how to rename keys, particularly:

• You can migrate . to _ and _ to __.
• Go, today, erases to camelcase. We could think about preserving _ and having . turn into camel-case otherwise.

Both of these example seem the opposite of what I would expect. __ seems like more separation than _ so I'd think . would become __ if anything, and I'm not sure I fully understand how the second one works. To me it seems like it would be better to CamelCase _ and turn . to _ if anything. My biggest issue there is that many languages already have casing conventions for constants so relying on case seems likely to cause issues there.

---

I was surprised to see 1.26 released with this known issue. Will there be a 1.26.1 to rectify it?

[dyladan]

Also affecting PHP codegen, for the same reasons as others: {{ attribute.fqn | to_const_name }} ends up with the same const name. For the time being, I can manually fix it by removing the deprecated one.

@brettmc keep in mind you're running into the situation mentioned above where users are going to have telemetry changed underneath them without realizing it. I'd caution against this.

[jsuereth]

1.26 was released assuming this is a codegen specific issue as we've made renames like this in the past. (see my apology above for making the decision, perhaps preemptively).

I still think this is an issue with codegen, but I'm asking the other semconv maintainers their opinion on backing off the change for now until a solution is found.

[jsuereth]

cc @open-telemetry/specs-semconv-maintainers

[dyladan]

I still think this is an issue with codegen, but I'm asking the other semconv maintainers their opinion on backing off the change for now until a solution is found.

It would be nice if this could be handled by codegen, but keep in mind that changing the way the codegen works is thorny for languages which already have released stable semconv packages. It means likely deprecating all old names and moving to the new style, which results in a lot of unneeded work in instrumentations to follow the new naming scheme.

[lmolkova]

It would be nice if this could be handled by codegen, but keep in mind that changing the way the codegen works is thorny for languages which already have released stable semconv packages. It means likely deprecating all old names and moving to the new style, which results in a lot of unneeded work in instrumentations to follow the new naming scheme.

Great point! It seems JavaScript is the only affected language. Given that it uses old tooling/templates and separates resource/other attributes into two different files, would it be fair to say that some breaking changes are inevitable there @dyladan ?

If so, this and other changes can be batched together and released as semconv v2 package.

---

Since (it seems) the cost of breaking is still low, I think we should disambiguate and make sure that different attribute are guaranteed to have different constant names.

The alternative I see is to tolerate the downside @trask brought up

the non-ideal part is that it will automatically change (some of) the emitted telemetry to a newer schema version while the instrumentation is still emitting an older schema version url

We should never rename a stable attribute and this would be a minor disturbances for experimental ones.

Still it might be surprising for users that their query no longer works even though the attribute constant name has not changed and I'd prefer to fix it if we can.

[dyladan]

It would be nice if this could be handled by codegen, but keep in mind that changing the way the codegen works is thorny for languages which already have released stable semconv packages. It means likely deprecating all old names and moving to the new style, which results in a lot of unneeded work in instrumentations to follow the new naming scheme.

Great point! It seems JavaScript is the only affected language. Given that it uses old tooling/templates and separates resource/other attributes into two different files, would it be fair to say that some breaking changes are inevitable there @dyladan ?

If so, this and other changes can be batched together and released as semconv v2 package.

This also affects PHP and Go at least. I suspect it also affects others. Separating resource/other attributes into separate files is an unrelated issue though. The issue is that we need both old and new names in order to handle the double-emit telemetry for the compatibility story.

JS is already planning to change how we generate semconv in the future (PR: https://github.com/open-telemetry/opentelemetry-js/pull/4690). We're going to keep the old names around and mark them as deprecated, but the new names are causing this problem. See https://github.com/open-telemetry/semantic-conventions/issues/1064 to see how we're generating the new names. I believe both the old and new generation scheme would have the same problems though.

Since (it seems) the cost of breaking is still low, I think we should disambiguate and make sure that different attribute are guaranteed to have different constant names.

The alternative I see is to tolerate the downside @trask brought up

the non-ideal part is that it will automatically change (some of) the emitted telemetry to a newer schema version while the instrumentation is still emitting an older schema version url

We should never rename a stable attribute and this would be a minor disturbances for experimental ones.

Still it might be surprising for users that their query no longer works even though the attribute constant name has not changed and I'd prefer to fix it if we can.

I'm not sure I agree that the cost of the break is "low" because the level of surprise would be quite high if we changed names out from under users without them making code changes.

My preferred fix would be to disallow any and all collisions, including with deprecated names, where non-alphanumeric characters are treated the same. For example messaging.client.id and messaging.client_id would be considered a disallowed collision.

[MrAlias]

Also affecting PHP codegen, for the same reasons as others: {{ attribute.fqn | to_const_name }} ends up with the same const name. For the time being, I can manually fix it by removing the deprecated one.

@brettmc keep in mind you're running into the situation mentioned above where users are going to have telemetry changed underneath them without realizing it. I'd caution against this.

In Go we release separate versions of semconv as separate packages. Dropping deprecated values would be acceptable for us in this situation given a user will need to explicitly make the upgrade by switching packages.

[lmolkova]

@dyladan If I understand your reply, we're talking about the same solution:

• No collisions in generated code. messaging.client.id and messaging.client_id should have different constant names. This would prevent future collisions.
• This would result in breaking changes to existing semconv libraries.
  • Most of them are still not stable and can do it.
  • JS plans some breaking changes and renaming HTTP_REQUEST_ORIGINAL_METHOD to HTTP_REQUEST_ORIGINAL__METHOD (and similar) could be done along with them.
  • PHP would need to do breaking changes to semconv package too. (thank you for pointing it out)

[jsuereth]

If this change affected a stable semconv library, I'd be very concerned for whomever declared that component stable -> Everything about this breakage is in unstable features:

The original attribute was unstable, the codegen package is unstable, etc.

[dyladan]

@dyladan If I understand your reply, we're talking about the same solution:

• No collisions in generated code. messaging.client.id and messaging.client_id should have different constant names. This would prevent future collisions.

• This would result in breaking changes to existing semconv libraries.

  • Most of them are still not stable and can do it.
  • JS plans some breaking changes and renaming HTTP_REQUEST_ORIGINAL_METHOD to HTTP_REQUEST_ORIGINAL__METHOD (and similar) could be done along with them.
  • PHP would need to do breaking changes to semconv package too. (thank you for pointing it out)

Yes that would be fine for us. I'd encourage the semconv/tooling group to fix this quickly if possible. The project to modernize JS semconv has already taken longer than expected (due to no fault of this group obviously).

If this change affected a stable semconv library, I'd be very concerned for whomever declared that component stable -> Everything about this breakage is in unstable features:

The original attribute was unstable, the codegen package is unstable, etc.

Yeah unfortunately we did a looong time ago (sept 30, 2021) as a part of our original tracing sdk stability (at the time all of our packages were versioned together) and it is the main reason we're so out of date on semconv, which is what we're trying to fix now. It's part of the reason we're deprecating all the old names (but keeping them around) and transitioning to a new naming scheme.

[dyladan]

To be clear, I wasn't saying the codegen change would be a problem for us, and it was probably a mistake to use the word stable. I was just saying if the way codegen works changes, all packages are changed and everyone needs to update to the new names. They may not be stable yet but it means likely every instrumentation needs to be updated in every language which I would think is not ideal.

[dyladan]

@dyladan If I understand your reply, we're talking about the same solution:

• No collisions in generated code. messaging.client.id and messaging.client_id should have different constant names. This would prevent future collisions.

• This would result in breaking changes to existing semconv libraries.

  • Most of them are still not stable and can do it.
  • JS plans some breaking changes and renaming HTTP_REQUEST_ORIGINAL_METHOD to HTTP_REQUEST_ORIGINAL__METHOD (and similar) could be done along with them.
  • PHP would need to do breaking changes to semconv package too. (thank you for pointing it out)

I think we're talking about different solutions. You're talking about changing the code generator. I was talking about a policy change which would disallow that collision from being made under the existing code generator.

[lmolkova]

This is a sample implementation for unambiguous constant names for Python: https://github.com/open-telemetry/opentelemetry-python/pull/3927

I can update build-tools (not sure if it's necessary since we're moving over to Weaver), but it's not required and can be done in lang-specific jinja templates in whatever format is idiomatic to the language.

One comment on the impl details: __ seems ugly, especially on the enums (e.g. MessagingSystemValues.AWS__SQS)

@dyladan

I was talking about a policy change which would disallow that collision from being made under the existing code generator.

We allow renaming experimental attributes on the spec level, I'd prefer to adjust code generator while we can and preserve the policy.

[jsuereth]

So far, most semconv maintainers I've heard from think that . vs. _ has meaning in our model. We may want to discuss this all together on the maintainers call or spec call, as it's possible this decision shouldnt be purely one group (I personally side with semconv).

Here's my expectation on how to progress:

• folks with semconv libraries that don't distinguish these two will need to continue releasing semconv from previous version or otherwise "freeze" that codegen.i assume JS is in the middle of that now.
• we can update our codegen tools (weaver and build gen) to provide helpers that preserve the meaningful non-alphanumeric characters we wish to preserve in semconv.
• generated code should update to these helpers over time.
• working groups struggling with semconv 1.26 should hold off on updating to 1.26 for tooling support. This release was actually primarily motivated by creating an attribute registry and enabling new tooling options anyway. In particular I expect codegen to be altered to match the actual semantics of the registry, including the rules of identity used by semconv tooling.

This situation is similar to the "enum in proto buf" issues where you can have changes to proto files that break generated code but not the protocol. I think we take a similar approach here

[dyladan]

Thanks for the quick responses. I think we probably should have a larger discussion and agree that _ vs . likely should be distinguished as meaningful in some way. I like the idea of letting languages have some control over the transformation in order to be more idiomatic.