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

Code generation fails for SEMCONV 1.19.0 #3299

Closed
marcalff opened this issue Mar 6, 2023 · 10 comments
Closed

Code generation fails for SEMCONV 1.19.0 #3299

marcalff opened this issue Mar 6, 2023 · 10 comments
Assignees

Comments

@marcalff
Copy link
Member

marcalff commented Mar 6, 2023

Using the latest spec 1.19.0 release.

Using build tools 0.17.0

Code generation for semantic conventions misses items in the generated code, leading to build breaks compared to version 1.18.0.

In particular, attributes like http.method got moved to common attributes by:

However, the code generation tool is not ready for this:

As a result, code generated with spec 1.19.0 + build tool 0.17.0 contains less constant definitions compared to 1.18.0, breaking the build because constants for http.method are missing:

This is not only blocking for 1.19.0, but will also affect every semantic convention changes done from now on, until the build tools are fixed so that code can be generated again from yaml files.

@lmolkova
Copy link
Contributor

lmolkova commented Mar 7, 2023

I'm not sure if build-tools#133 is related to the issue, but I'm looking into build-tools fix for code generation.

@lmolkova
Copy link
Contributor

lmolkova commented Mar 7, 2023

The problem is in #3188 and #3190 which reuse resource attributes in trace semconv and vice versa.

Without these two, java templates work just fine. With them they show a bunch of errors.

I believe Java templates work a bit better because they allow taking attributes from everywhere except resources (--exclude resource/**) and cpp does this: --only span

I created this issue open-telemetry/build-tools#155 and should be able to work on it in a few days.

@lmolkova
Copy link
Contributor

lmolkova commented Mar 20, 2023

@tsloughter, @marcalff, and everyone else affected, would you mind trying new build-tools version - 0.18.0- with the fix? (thanks for @Oberon00 and @arminru for the help!)

It will be necessary to change the generation script to specify which signals to generate using --only flag. It's also important to point source to the root folder so that referenced attributes are resolved everywhere

Here's the sample of changes in Java repo:

docker run --rm \
  -v ${SCRIPT_DIR}/opentelemetry-specification/semantic_conventions:/source \
  -v ${SCRIPT_DIR}/templates:/templates \
  -v ${ROOT_DIR}/semconv/src/main/java/io/opentelemetry/semconv/trace/attributes/:/output \
  otel/semconvgen:$GENERATOR_VERSION \
-  --exclude resource/** \
+  --only span,event,attribute_group,scope \
  -f /source code \
 ...

Resources:

docker run --rm \
- -v ${SCRIPT_DIR}/opentelemetry-specification/semantic_conventions/resource/:/source \
+ -v ${SCRIPT_DIR}/opentelemetry-specification/semantic_conventions:/source \
  -v ${SCRIPT_DIR}/templates:/templates \
  -v ${ROOT_DIR}/semconv/src/main/java/io/opentelemetry/semconv/resource/attributes/:/output \
  semconvgen:$GENERATOR_VERSION \
+ --only resource \
  -f /source code \
....

@tsloughter
Copy link
Member

Hm, I'm getting gen-semconv: error: unrecognized arguments: --only resource with 0.18.0

@tsloughter
Copy link
Member

Hm, it works if its the first argument.

@Oberon00
Copy link
Member

Oberon00 commented Mar 21, 2023

Yes, this is a bit of a quirk of Python's argparse: --only is an argument to the program itself, not to the code (or markdown) subcommand, so you need to specify it before the subcommand (similar to how you need to specify the -v to docker before the image name).
(There would actually be a workaround we could implement in the generator tool: we could add all the common arguments https://github.com/open-telemetry/build-tools/blob/2e9bfeebd327d14a5c5c3daece9b48f756b78cb7/semantic-conventions/src/opentelemetry/semconv/main.py#L223-L257 additionally to all subcommands; however that also makes the --help output weird then)

@marcalff
Copy link
Member Author

@tsloughter, @marcalff, and everyone else affected, would you mind trying new build-tools version - 0.18.0- with the fix? (thanks for @Oberon00 and @arminru for the help!)

Thanks for the fix.

It works, see C++ PR:

@marcalff
Copy link
Member Author

Before closing this issue for opentelemetry-specification, please consider to add a CI check, to enforce that semantic conventions always build after a spec semconv change in yaml files.

Credits for the idea to @lmolkova , see open-telemetry/build-tools#156 (comment)

@marcalff
Copy link
Member Author

There is no longer a point in implementing a CI check in the specification repository,
as semantic conventions moved to their own repo.

Closing this issue.

@Oberon00
Copy link
Member

I can reopen & move it to the semconv repo instead if you prefer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants