Closed dyladan closed 1 week ago
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.
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
I'd be interested to see if this affects other language code generators
Yeah I think it's likely cc @jack-berg
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?
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
This looks to be affecting the Go code generation: https://github.com/open-telemetry/opentelemetry-go/actions/runs/9180409074/job/25244759934?pr=5394
Previously, when renaming of this variety we DROPPED the old attribute. Now we do not.
I see a few paths forward here:
.
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..
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..
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.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.
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.
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:
- 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.
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
We had some discussion on this in the semconv tooling group. I think there's a few options for how to rename keys, particularly:
.
to _
and _
to __
._
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.
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 _
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.
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?
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.
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.
cc @open-telemetry/specs-semconv-maintainers
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.
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.
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.
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.
@dyladan If I understand your reply, we're talking about the same solution:
messaging.client.id
and messaging.client_id
should have different constant names. This would prevent future collisions.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 If I understand your reply, we're talking about the same solution:
- No collisions in generated code.
messaging.client.id
andmessaging.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.
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 If I understand your reply, we're talking about the same solution:
- No collisions in generated code.
messaging.client.id
andmessaging.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.
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.
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:
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
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.
The change from messaging.client_id
> messaging.client.id
looks like "just a rename" but it's more than that. We identified that there can be more attributes under messaging.client
, so it wasn't rename, it rather became a namespace.
I'm highlighting this because these sort of situations will probably only happen rarely when stabilizing things. Additionally, I think there's a breach of boundaries when individual languages can dictate what's allowed vs not allowed in semconv. For ex, I would not think it's good that semconv is blocked to move an attribute to a namespace like this case, because it may cause conflicts in some languages.
I agree with @AlexanderWert others. Dot .
and underscore _
have important meaning in semconv and codegen. Using those interchangeably is not a good alternative I believe. It is also worth mentioning, this is also part of the attribute naming spec: https://opentelemetry.io/docs/specs/semconv/general/attribute-naming
For each multi-word dot-delimited component of the attribute name separate the words by underscores (i.e. use snake_case). For example http.response.status_code denotes the status code in the http namespace.
My personal opinion (and I see others with the same) is that _
is a good mapping for the namespace denominator (.
) when generating code. I think what is missing for us to figure it out is how to map multi-word (_
) attribute names.
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.
I think going forward, we SHOULD NOT use _
in codegen for multi-word attributes as that makes it impossible to distinguish what is a namespace vs attribute name in a const. This now also depends on the language, as said in the thread, it can map _
to ` and use pascal/camel cases. Other alternative is stick it all together, like
MESSAGING_CLIENTID`
The latter solves the problem with JS, but the former won't solve problems for languages that already published version with removing _
and using things like pascal case.
I also wonder if this isn't best left to each SIG to define the code gen template however is most practical to them? I'm not even sure if it makes sense for semconv to say like: namespace separator when doing code gen is _
, multi-word name separator is something
, because it may not be ideal/idiomatic to all languages.
I think there's a breach of boundaries when individual languages can dictate what's allowed vs not allowed in semconv. For ex, I would not think it's good that semconv is blocked to move an attribute to a namespace like this case, because it may cause conflicts in some languages.
There is some level of responsibility to ensure the semconv can be reasonably consumed by code generators. For example, there aren't attributes that differ only in casing. The collision happens when you use the tooling provided by semconv in the way suggested by semconv, which is why it is affecting several languages.
I agree with @AlexanderWert others. Dot
.
and underscore_
have important meaning in semconv and codegen. Using those interchangeably is not a good alternative I believe. It is also worth mentioning, this is also part of the attribute naming spec: https://opentelemetry.io/docs/specs/semconv/general/attribute-naming
I agree in theory, but semconv uses characters which are not commonly allowed in variable names (.
). When mapping to a more restrictive character set, some collisions are not unexpected.
I also wonder if this isn't best left to each SIG to define the code gen template however is most practical to them? I'm not even sure if it makes sense for semconv to say like: namespace separator when doing code gen is
_
, multi-word name separator issomething
, because it may not be ideal/idiomatic to all languages.
I think if you leave it to individual languages, you're likely to end up with situations like this again. Semconv should provide at the very least some strong guidelines.
There is some level of responsibility to ensure the semconv can be reasonably consumed by code generators. For example, there aren't attributes that differ only in casing. The collision happens when you use the tooling provided by semconv in the way suggested by semconv, which is why it is affecting several languages.
Yes, agreed, it's unfortunate that the tooling did treat .
and _
as the same thing which causes this now. But it seems to me that's exactly the reason why - the codegen tooling took an option and gave to everyone which should have been better picked by the language and generate what's better there. Like you said, .
is not allowed in variable names, and other languages don't use _
for separating things and rather use classes/enums or PascalCase. So it's hard for the tooling to do this and should be left to SIGs I think.
That doesn't fix the situation of course, and I'm not sure even how we should fix it now 🤔
@joaopgrassi I actually think we'll want to create new "XYZcase" filters that can handle semconv's .
and `` appropraitely in our tooling. cc @lquerel @lmolkova
Sure I think that's something we need anyway, but not sure how that will fix the problems that @dyladan brought it up? The only way I see to unblock, is that the new generated name somehow is different.
@joaopgrassi No - we still need to address the fact that previous codegen did NOT respect the differences between .
and _
.
My suggestion for that is we'll need to migrate users from codegen that did NOT respect it to codegen that does, possibly via new packages, distributions, etc.
E.g. you could take your existing semconv library and "lock it down" at 1.25.0
.
Because of this - I think we (@open-telemetry/specs-semconv-maintainers and codgen owners) have a responsibility to take some time and provide a codegen solution that won't face an issue like this again in the near/immediate future. E.g. we should also make sure we provide guidance around: https://github.com/open-telemetry/semantic-conventions/issues/1064.
I believe we need to:
to_const_name
and others (if any) don't generate ambiguous constant names. Language SIGs can use them or can still write their own helpers with Jinja. .
-> _
or whatever convention we'll pick) as a best effort to stay consistent when helpers are not usedto_const_name
in jinjaAs a stretch goal I wish we could add a code generation sample + sanity check into semconv repo. This is one of the big feedback points we received in #551.
How it could work:
This seems doable (as long as we pick language with good existing API compat tooling)
Opentelemetry-cpp was affected also:
Generation for the old name was disabled in the template.
{#
MAINTAINER:
semconv "messaging.client_id" is deprecated
semconv "messaging.client.id" is to be used instead
Now, because we use k{{attribute.fqn | to_camelcase(True)}},
both names collide on C++ symbol kMessagingClientId.
Do not generate code for semconv "messaging.client_id"
#}
Adding to this:
I started writing a codegen for C# and I'm running into the same issue. The only way around it given the information at the time of rendering the template is to disable rendering of any deprecated attributes to avoid name clashes.
@alxbl @marcalff
it should be possible to modify the function that generates constant name. It will affect existing attributes, other than messaging.client_id
, but it will prevent future collisions. They are likely to happen.
The tooling will provide the proper function for it, so this would be a workaround.
What's important is to agree on the consistent formatting.
For languages that use camelCase or PascalCase it could probably be formatted as MessagingClient_Id
(for messaging.client_id
) and MessagingClientId
(for messaging.client.id
)
it can be achieved with existing tooling with a macro similar to
{%- macro to_const_name_v2(attr_name) -%}
{%- set ns=namespace(up=True) -%}
{%- for l in attr_name -%}
{%- if ns.up -%}
{{l | upper}}
{%- elif l != '.' -%}
{{l}}
{%- endif -%}
{%- set ns.up=(l=='.' or l=='_') -%}
{%- endfor -%}
{%- endmacro %}
In any case, please do share your thoughts on the format we should provide in tooling (whether MessagingClient_Id
or HttpRequestMethod_Original
) is reasonable or you'd prefer some other format that prevents collisions like this
I think this needs more investigations.
For example, there are collisions between foo.barbaz
and foobar.baz
too.
For example, there are collisions between foo.barbaz and foobar.baz too.
They will result in different names: FooBarbaz
and FoobarBaz
. Which is not great, but not a collision in most languages.
The alternative is to do something like .
-> _
and _
-> __
. I.e. Messaging_Client_Id
and Messaging_Client__Id
(which seems to work better for languages that use snake_case or SCREAMING_SNAKE_CASE for constants)
Note this also affect class names
user_agent.*
(User_AgentAttributes
) should be distinguishable from useragent.*
(UserAgentAttributes
).db.cassandra.consistency_level
enum (DbCassandraConsistency_LevelValues
) should be distinguishable from db.cassandra.consistencylevel
(DbCassandraConsistencyLevelValues
)For example, there are collisions between foo.barbaz and foobar.baz too.
They will result in different names:
FooBarbaz
andFoobarBaz
. Which is not great, but not a collision in most languages.The alternative is to do something like
.
->_
and_
->__
. I.e.Messaging_Client_Id
andMessaging_Client__Id
(which seems to work better for languages that use snake_case or SCREAMING_SNAKE_CASE for constants)
Correct, I missed the fact that the .
is implicitly represented by an uppercase in camel case.
So, to summarize:
Semantic conventions:
messaging.client_id
messaging.client.id
can be generated as:
kMessagingClient_Id
kMessagingClientId
or as:
MESSAGING_CLIENT__ID
MESSAGING_CLIENT_ID
depending of the language style (CamelCase, UPPERCASE).
This is a breaking change for every semantic convention that contains a _
character, but seems viable in the long term to prevent collisions.
The breaking change can not be avoided, by definition: the mapping for one of the colliding names has to change.
@lmolkova This solution will work for us (opentelemetry-cpp).
@lmolkova
Assuming this is satisfactory for all SIG, could we have a new release of https://github.com/open-telemetry/build-tools, so that the primitives that convert names are adjusted (or new primitives are provided) ?
Then each SIG can use the fixed primitives to generate code that disambiguates collisions.
cc @open-telemetry/cpp-maintainers
it can be achieved with existing tooling with a macro similar to
[...code...]
Thanks! I will use this for the time being until this is fixed in build-tools.
I agree with @marcalff that the to_const_name
or to_xyzCase
methods should not replace _
with .
before doing the conversion, that way we avoid the collisions and as described in his post.
Discussed at the SemConv and maintainers SIGs:
_
as in @trask proposal https://github.com/open-telemetry/semantic-conventions/issues/1031#issuecomment-2125013562: messaging.client_id
-> MESSAGING_CLIENTID
and MessagingClientid
_
.The recommendation for messaging.client_id
-> messaging.client.id
would be to drop the old attribute.
Motivation:
Example on how to implement configurable dropping in Jinja - https://github.com/crossoverJie/semantic-conventions-java/pull/1/files
See https://github.com/open-telemetry/semantic-conventions/issues/1118#issuecomment-2173803006 for discussion on the general issue (and steps we're taking to prevent future collisions).
closing this one: see https://github.com/open-telemetry/semantic-conventions/issues/1031#issuecomment-2173887975 for client_id specific guidance and https://github.com/open-telemetry/semantic-conventions/issues/1118#issuecomment-2173803006 for the future approach.
For the time being such changes are prohibited and guarded with a policy check in CI #1209
Area(s)
area:messaging
What happened?
Last week in https://github.com/open-telemetry/semantic-conventions/pull/948
messaging.client_id
was renamed tomessaging.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 nameMESSAGING_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.