Weaver registry diff: docs and usability suggestions

[lmolkova]

Some suggestions from working on open-telemetry/semantic-conventions#2307.

Take a look at the

weaver/docs/usage.md

Lines 419 to 427 in 19a6c7e

	--diff-format <DIFF_FORMAT>
	Format used to render the schema changes. Predefined formats are: ansi, json, and markdown
	[default: ansi]
	--diff-template <DIFF_TEMPLATE>
	Path to the directory where the schema changes templates are located
	[default: diff_templates]

The only way to understand how templates should look like is by reverse engineering the https://github.com/open-telemetry/weaver/tree/main/defaults/diff_templates folder structure

1. Let's assume I passed non-existed folder:

   weaver registry diff -r  https://github.com/open-telemetry/semantic-conventions/archive/refs/tags/v1.34.0.zip[model] \
   --baseline-registry https://github.com/open-telemetry/semantic-conventions/archive/refs/tags/v1.33.0.zip[model] \
   --diff-template ./does-not-exist --diff-format yaml -o diff

   Expected: error - folder not found
   Actual: success with full diff as if I didn't provide --diff-template

2. Let's assume I passed existing folder name, but folder structure is not the right one and there is no yaml/weaver.yaml - same result as in p1

---

Suggestions:

1. if --diff-template is provided and it does not exist, fail - don't fallback to default one

2. I really don't understand how --diff-template is different from --config and why do I need to pass it along with --diff-format - i'm providing my own templates and my own config - just let me pass the path to weaver config and call it a day.

   I.e. let me do

   - weaver registry diff -r  ./current  --baseline-registry ./baseline --diff-template ./path/to/folder-with-specific-structure --diff-format yaml
   + weaver registry diff -r  ./current  --baseline-registry ./baseline --config ./path/to/my-weaver-config.yaml

3. Add some logs to the template engine like reading ./path/to/config/weaver.yaml, executing jq, running jinja, etc

[lquerel]

I must admit that there are indeed quite a few inconsistencies between weaver generate and weaver diff.

Note: I added in a comment further down a series of use cases and examples of what Weaver would look like with the changes mentioned in this proposal.

Current Status

weaver generate

The main parameters of the weaver generate command are:

• --templates: this named parameter is used to specify the root directory of the target folders containing the templates.
• --param and --params: these named parameters are used to configure the templates from the command line.
• --config: this named parameter is used to override the weaver.yaml file located in the target template directory.
• target: this positional parameter is used to define the target to generate, for example: java, go, rust, … This target is converted into a subdirectory of the root directory pointed to by --templates.
• output: this optional positional parameter allows specifying a destination directory where the generated artifacts will be produced.

weaver diff

• --diff-template: this named parameter serves the same purpose as the --templates parameter described above.
• --diff-format: this named parameter serves the same purpose as the target positional parameter described above.
• --output: this named parameter serves the same purpose as the output positional parameter described above.

With the diff command, it is currently not possible to specify --param, --params, and --config.

Other Sources of Confusion and Considerations

It has often been mentioned that organizing templates by target is not intuitive. Supporting multiple targets is an advanced use case, yet unfortunately it’s the default favored by Weaver.

However, once we have the out-of-the-box templates integrated into Weaver, I would find it useful to simplify our users’ experience as much as possible, since we can expect this to cover 90% of use cases. So ideally, I’d like to support something as simple as:

• weaver generate java
• weaver generate go
• ...

In this case, the concept of a target makes perfect sense.

In a way, we already have out-of-the-box templates for the diff command, but I wasn’t foresighted enough to use consistent syntax. Ideally, we should have something like weaver diff markdown, weaver diff json, … and reserve the --templates parameter for custom use cases.

Proposal

Preamble: With the goal of minimizing impact on current users, I believe we can agree that it is preferable to align with the approach used in the generate command where it makes sense, since it is the most used command.

I propose the following adaptations:

• Unification of parameter names and semantics between the two commands:
  • Rename --diff-template to --templates (only impact the diff command)
  • Transform the named parameter --diff-format into the target positional parameter (only impact the diff command)
  • Unified use of --param and --params for both commands (only impact the diff command)
• Make --templates and the target positional parameter mutually exclusive. When --templates is specified, Weaver will consider that the directory provided points directly to the folder containing the templates and the weaver.yaml file, and not to a parent directory with subfolders organized by target.
• Introduce a new parameter --root-targets, also mutually exclusive with --templates, to redefine the directory currently used for the target concept.
• Rename --config to --override-config. This parameter is for advanced use cases, and I doubt many people use it currently. The idea behind it is to allow overriding the weaver.yaml file included with the templates from the command line. This is useful for “remote” templates where one might want to override the template author’s config file for specific local reasons.
• Removal of the named parameter --output from the diff command to follow the same logic of using a positional parameter as in the generate command.
• Regarding positional parameters, the behavior changes slightly:
  • If --templates is specified, then the target positional parameter is no longer present, and the first positional parameter becomes output (which remains optional).
  • When the --templates parameter is not specified, the behavior remains as it is today: the first positional parameter is target and the second, optional one, is output.

Once we have out-of-the-box templates, the concept of a target will play a central role. It will serve as a short name to designate what needs to be generated without requiring knowledge of the template location. Like said before we will have something like: weaver generate java, weaver generate rust, weaver diff markdown, ...

The --templates parameter will then become more of an advanced option, allowing users to define their own custom templates (local or remote). This parameter will directly point to the directory (or a remote location) containing the templates, removing the need for the current hierarchy and thus eliminating one of the confusion sources mentioned by @lmolkova and Martin.

It is also important to understand that the weaver.yaml file is optional. It can be inherited from the home directory or even overridden by a local file specified on the command line (see previous discussion on --override-config). Again, this concept of overriding weaver.yaml files is for advanced use and only makes sense in enterprise use cases where default templates might be provided, with configurations that can be overridden for specific teams/products.

Finally, in the command documentation (--help), I propose annotating certain parameters like --override-config, --root-targets, ..., with a phrase such as "(advanced usage)".

With this proposal, I believe we achieve consistency between the generate and diff commands, and I think the impact on users of the generate command remains minimal and understandable.

[lmolkova]

I believe there are 3 scenarios behind this proposal:

1. weaver generate java - future, opinionated, easy, high-level
2. weaver generate --full-path-to-config - current, non-opinionated, difficult, low-level [updated per offline discussion]
3. weaver generate --templates - something in between - you can bring your templates, but there is a default config [updated per offline discussion]

First I suggest to drop scenario 3 - it's something in between and we can always build it later.

Everything we do in semconv and codegen across otel today needs p2 - custom weaver.yaml and custom templates.
We can think about scenario 1 as an opinionated wrapper around scenario p2.

My suggestion is to make p2 easy to use since it's the one we actually use today.

[lmolkova]

from offline discussion:

Having weaver.yaml being optional in --templates dir is an error-prone pattern. Let's say I misspell file name and called it waever.yml - it will be ignored. I believe #761 p2 is exactly this problem.

[lquerel]

Ok, let’s take a few examples associated with use cases and apply what is presented in this proposal.

Use case 1

I am a standard Weaver user and I want to generate Java code for my semconv as well as documentation.

weaver registry generate java
weaver registry generate markdown

This requires having out-of-the-box templates pre-integrated into Weaver. We’re not there yet, but it’s one of our goals and we have the underlying technical infrastructure to do it (the concept of embedded defaults, virtual folders, and targets).

What could be simpler than this for an occasional user?

Use case 2

I am a standard Weaver user and I would like to use someone else’s templates that have been placed in a GitHub repo.

weaver registry generate --templates https://github.com/user-xyz/weaver-python-templates.git

Note: I’m not specifying a configuration file here, but a GitHub repo containing the templates and the accompanying configuration. By pointing to the git repo or to a directory within the git repo, things are simple. Weaver downloads the repo and interprets the templates and the config file as if they had been defined locally. If we pointed directly to a config file (e.g., https://github.com/user-xyz/project.git[weaver/templates/weaver.yaml]) and the config file internally pointed to templates that could be anywhere in the repo, then the download mechanism in Weaver would be much more complicated and more fragile.

Use case 3

I am an advanced user and I would like to define my own templates for a language not covered by OTEL.

weaver registry generate --templates <local-folder> 

This command is equivalent to the previous one, except that we are switching from a git URL to a local directory.

Use case 4

I am an advanced user and I would like to define my templates to leverage the results of the diff command and pass few parameters in command lines.

weaver registry diff --templates <local-folder> --param param1=value1

Use case 5

I am an advanced user and I would like to be able to import templates from a git repo while overriding the default configuration.

weaver registry generate --templates https://github.com/user-xyz/weaver-python-templates.git  --override-config <local-weaver-yaml>

Additional comments

Weaver’s behavior in the absence of a weaver.yaml file can be improved to eliminate the issue mentioned earlier in one of Liudmila’s comments.

[lmolkova]

I'm not sure it's possible to eliminate the absence of weaver.yaml issue - if I just misspelled the filename, weaver has no means to understand my intent and will generate something from the default one.

From the high level perspective, I don't see why someone would want default weaver.yaml if they have custom templates.
Now we require every low-level user (all existing ones) to put a file with specific file name inside that dir or pass --config.

So I would still suggest to collapse 2-4 to

weaver registry diff --config  https://github.com/user-xyz/weaver-python-templates/blob/v1.34.0/path/to/weaver-stable.yml  --param param1=value1

(with optional param)

I think having one explicit parameter results in a better user experience, i.e. fewer questions and mistakes. It's a separate question whether the cost of implementation is too high.

[lquerel]

@lmolkova Regarding the absence of the weaver.yaml file, Weaver could do a better job by 1) making the presence of weaver.(yaml | yml) mandatory (it's a minor change), and 2) displaying an error message if it’s missing. This would have prevented the issue you mentioned in p1 of the initial issue description.

With your approach, how do you address all the notes from my use case 2?

[lmolkova]

Regarding the absence of the weaver.yaml file, Weaver could do a better job by 1) making the presence of weaver.(yaml | yml) mandatory (it's a minor change), and 2) displaying an error message if it’s missing

Awesome, so it seems we agree that we need to require weaver.yaml and don't ever default to one except use-case 1 - weaver registry generate java.
Still --config would be more robust and common way to pass config, right?

With your approach, how do you address all the notes from my use case 2?

1. do we have to support arbitrary urls?
2. If we really need it:
   a) simple 'crawl' step would be to require templates to sit under the same parent as weaver.yaml for the time being
   b) implement the complexity of downloading relative paths
   c) add advanced params for this (I argue) advanced edge case. You can provide both - path to yaml and path to templates and they could be different and in different base urls.

Today, all our use cases are local files, so I'd optimize for it and would start with 2a - limited path resolution for templates

[jsuereth]

Maybe we can agree on some simple principles?

Unification of parameter names and semantics between the two commands

Agree on this, but then the immediately following points seem to keep some divergence. We should justify when something isn't the same WHY and make sure this is a good version. This includes allowing positional vs. named arguments. I'm honestly fine if we ONLY allow named arguments going forward and require strong justification for positional arguments, e.g.

Here's what I think I hear @lquerel and @lmolkova saying and want to confirm:

• When there's a configuration option useful for a component (e.g. template engine params) and that component is used in a weaver command, we should use the same argument and type (positional vs. named) for the CLI.
• We want the ability to point at a specific template config the user has locally created.
  • Pointing at a directory with a weaver.yaml seems like a good, consistent option here.
  • We want to point directly at the directory, not some layered option.
  • This is the thing MOST users of weaver are trying and getting confused over.
• We need the ability to filter to only generate a portion of templates defined in weaver.yaml
  • This is likely NOT a single jinja file.
  • This should be a more advanced option.
  • I don't think @lmolkova really wants or needs this right now.
• We need the ability to provide out of the box codegen that's easy to reference and use.
  • We'd like something like java or go to represent a good java or go codegen default template. Similarly for ansi for error output.
  • We need enough "smarts" for weaver not to throw, e.g. weaver diff output at a codgen template. At a minimum this should lead to a good error message.
  • The current design for this makes the goal of pointing at specific configuration (directory with weaver.yaml) hard.

If we agree on these principles, I think the design options can fall out from there.

[lquerel]

@jsuereth

We want the ability to point at a specific template config the user has locally created.

• Pointing at a directory with a weaver.yaml seems like a good, consistent option here.
• We want to point directly at the directory, not some layered option.
• This is the thing MOST users of weaver are trying and getting confused over.

So I conclude that you prefer the --templates approach, which points directly (i.e. without an intermediate level) to the directory containing the templates and the weaver.yaml file.

I also agree with the rest of what you pointed out.

[lmolkova]

We want the ability to point at a specific template config the user has locally created.

• Pointing at a directory with a weaver.yaml seems like a good, consistent option here.
• We want to point directly at the directory, not some layered option.
• This is the thing MOST users of weaver are trying and getting confused over.

I suggest to go one step further and ask user to provide a path to a config file instead of directory where its located.
The config file should have a path or link to local/external templates.

This gives:

• single source of all configuration options
• no hidden defaults (weaver.yaml|yml)
• typical CLI interface

--templates dir without layers option is reasonable, but not awesome. As a user I was/am confused on:

• what should I have inside this folder?
• what the name of the config file should be?
• can I have more than one config there?
• can I keep my templates and weaver.yaml in different places?

I think I know 75% of the answers now, but every time I assume, @lquerel tells me I assumed wrong. So the key principle I advocate for is being fully explicit with config for the advanced case (bring your own config and templates), so nobody needs to assume.