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

Need a soonish fix for current usage of code generation #242

Closed
AlexanderWert opened this issue Nov 28, 2023 · 9 comments · Fixed by #243
Closed

Need a soonish fix for current usage of code generation #242

AlexanderWert opened this issue Nov 28, 2023 · 9 comments · Fixed by #243
Assignees
Milestone

Comments

@AlexanderWert
Copy link
Member

  • In the past the way build-tools were used for code generation (in many language SDKs) assumed a strict separation between resource attributes and semantic attributes
  • This assumption is not true anymore, as there are attributes that can be used as both, resource and semantic attributes
  • Currently, there's no (easy/at all?) way to use the build-tools for code generation in a way that the above separation applies if we have namespaces in the registry that would contain both, resource and semantic attributes
  • With the migration to a registry-based attributes model, the current limitation

The above leads to the fact that some work on semantic conventions is blocked, because we cannot introduce namespaces with a mix of resource and semantic attributes.

@AlexanderWert
Copy link
Member Author

\cc @lmolkova FYI: I created this issue to track all ongoing semconv PRs that are blocked by this issue.

@joaopgrassi
Copy link
Member

Given #243, do we still need anything else to close this? @AlexanderWert

@lmolkova
Copy link
Contributor

lmolkova commented Feb 12, 2024

Adding a list of remaining things not yet addressed in #243

Critical, needed to merge to main and release:

Can be done as separate PRs, not critical for build-tools release:

Done, needs approvals:

Needs discussion:

@jsuereth
Copy link
Contributor

jsuereth commented Feb 14, 2024

I created a prototype to help with filenames: feature/codegen-by-namespace...jsuereth:build-tools:wip-filename-choices

TL;DR; is we can allow --output filename to be a JINJA2 template itself and provide different versions of the "prefix" for the file to the template,e .g. camel-case, snake-case, ....

@joaopgrassi
Copy link
Member

joaopgrassi commented Feb 14, 2024

I created a prototype to help with filenames: feature/codegen-by-namespace...jsuereth:build-tools:wip-filename-choices

TL;DR; is we can allow --output filename to be a JINJA2 template itself and provide different versions of the "prefix" for the file to the template,e .g. camel-case, snake-case, ....

LGTM :shipit:

#244

I merged that one now.

@lmolkova
Copy link
Contributor

I believe we have just two items left, but I don't consider them blocking:

  • don't create file if template filtered out all attributes - see this for the details:

    • We have means to not create empty files, it's just not super-convenient.
    • If we implemented it, it would not change generated code in any way, but implementation is not trivial.
  • automate updating Schema URLs list (see this on python and this on java):

    • This is something we can't achieve with Jinja templates. Also it's easy to do manually (add new constant to the list of schema urls when generating new semconv). Java also added check for it.

So the only thing before we start merging to main is getting green light from Java and Python - I'll check with Python tomorrow.

@jsuereth
Copy link
Contributor

Regarding This point:

  • don't create file if template filtered out all attributes - see this for the details

I wasn't sure how to make progress. IIUC, the filter is something applied by the template. Should we make filter more first class in the tooling? Specifically:

  • We provide filtered_attributes to the template
  • When filtered_attributes is empty, we do not ask to write the file

@lmolkova
Copy link
Contributor

lmolkova commented Feb 26, 2024

@jsuereth That's what I was also thinking.

Currently filter is fully controlled by the template and python code does not know, so can't prevent empty files.
At the same time, we can make filters first-class citizens (e.g. stable, experimental). This would also save a tiny bit of effort in jinja templates.

If we did this, we could reuse existing --only cmd arg which essentially applies filter on signal. We could change it to accept stability filters like codegen --only metrics,stable or codegen --only experimental.

With codegen --only metrics,stable SIGs would need to run codegen script for each combinations they want.
With codegen --only experimental SIGs would likely have more filters inside the template (for signals) and they'd need to take care of empty files on their own.

I feel this feature (at least how I see it) brings diminishing returns or just needs more thought than I put into it.

@lmolkova lmolkova self-assigned this Mar 12, 2024
@lmolkova lmolkova added this to the 0.24.0 milestone Mar 12, 2024
@lmolkova
Copy link
Contributor

I believe this can be closed - we got configuration from Java and Python SIGs that codegen works well. If there are any further improvements, we can track them in new issues. Thanks everyone!

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

Successfully merging a pull request may close this issue.

4 participants