Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

to_const_name generates invalid variable identifiers #59

Closed
legendecas opened this issue Aug 10, 2021 · 3 comments · Fixed by #64
Closed

to_const_name generates invalid variable identifiers #59

legendecas opened this issue Aug 10, 2021 · 3 comments · Fixed by #64
Assignees
Labels
enhancement New feature or request semconv/model Related to the data model or YAML format of the semantic convention generator semconv Related to the semantic convention generator.

Comments

@legendecas
Copy link
Member

legendecas commented Aug 10, 2021

Recently in semantic conventions, two new types of id were introduced:

The const names generated for those ids are ALIBABA CLOUD and 1XRTT respectively. Those generated values are not valid variable identifiers in most languages.

In opentelemetry-js, we use to_const_name to generate keys for those ids: https://github.com/open-telemetry/opentelemetry-js/blob/main/scripts/semconv/templates/SemanticAttributes.ts.j2#L57. In this case, the keys should be valid variable identifiers.

Should we change the semantic conventions or update the generator to generate valid identifiers?

@Oberon00
Copy link
Member

I think we don't have any spaces in the semantic conventions so far, so my preferred solution would be to change alibaba cloud to use an underscore, like all other comparable names.

I would prefer the same for 1xRTT. There is a reason we have separate "value" and "id" fields in the YAML.

@Oberon00
Copy link
Member

open-telemetry/opentelemetry-specification#1863 will fix the original issue, let's leave this open to add validation/linting.

@Oberon00 Oberon00 added enhancement New feature or request and removed bug Something isn't working labels Aug 11, 2021
@Oberon00 Oberon00 self-assigned this Sep 8, 2021
@Oberon00
Copy link
Member

Oberon00 commented Sep 8, 2021

I'm working on this.

@Oberon00 Oberon00 added the semconv/model Related to the data model or YAML format of the semantic convention generator label Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semconv/model Related to the data model or YAML format of the semantic convention generator semconv Related to the semantic convention generator.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants