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

registry resolve does not work with local repositories #189

Closed
MadVikingGod opened this issue May 30, 2024 · 5 comments
Closed

registry resolve does not work with local repositories #189

MadVikingGod opened this issue May 30, 2024 · 5 comments

Comments

@MadVikingGod
Copy link
Contributor

Describe the bug
When working with a local copy of the opentelemetry semantic convention trying to do weaver registry resolve will fail with the error:

⠇ Resolving registry `../semantic-conventions`Diagnostic report:

  × The semantic convention spec is invalid (path_or_url: "../semantic-conventions/templates/registry/markdown/
  │ weaver.yaml"). unknown field `templates`, expected `groups`

To Reproduce
Steps to reproduce the behavior:

  1. Checkout semantic convention. Make sure it's on main (also fails on tag v1.26.0)
$ git switch main                                 
Switched to branch 'main'
$ git pull 
Already up to date.
  1. Resolve registry
$ cd ../weaver   
$ target/debug/weaver registry resolve -r ../semantic-conventions -d model  
⠇ Resolving registry `../semantic-conventions`Diagnostic report:

  × The semantic convention spec is invalid (path_or_url: "../semantic-conventions/templates/registry/markdown/
  │ weaver.yaml"). unknown field `templates`, expected `groups`


Total execution time: 0.020492865s

Expected behavior
This should resolve the registry and output to stdout.

Additional context
This work on tag v1.25.0

@lquerel
Copy link
Contributor

lquerel commented May 30, 2024

This is probably because you should point to the model sub-directory which contains the effective semconv YAML files. For a remote git registry, there is a parameter --registry-git-sub-dir that defaults to model (this applies only to git-url and not to local directory). This parameter exists because there is no well-accepted git URL notation to specify a sub-folder. However, for a local registry, we can directly specify the right folder, which is why there is no specific parameter for it.

Please close this GH issue, if my assumption is correct.

@MadVikingGod
Copy link
Contributor Author

I see it does work that way, but it is confusing to use.

Could we either:

  1. Allow the directory option to work with both git and local directories, so there is consistency between the configuration.
  2. Add examples to the --help to point out the differences, such as:
Examples:
https://github.com/open-telemetry/semantic-conventions.git
../semantic-conventions/model

@lquerel
Copy link
Contributor

lquerel commented May 31, 2024

I am including @jsuereth to get his feedback on this. Personally, I would lean towards an improvement in the help for the following reasons:

  • The fact that the official registry places the semconv files in the model subdirectory is an arbitrary choice by OpenTelemetry. I think there is nothing that requires a different organization for private/vendor registries.
  • The second parameter --registry-git-sub-dir exists only because there is no official git URL syntax to specify a subdirectory within a repo.
  • For a local registry, the problem does not arise as one can directly point to the desired subdirectory. Thus, having a second parameter to specify a subdirectory of a local directory will probably be perceived as strange.

The alternatives I see:

  1. Keep the current approach, but improve --help to clarify usage and improve the error message by reminding users to specify the directory containing the semconv files.
  2. Remove --registry-git-sub-dir and support an extended syntax for git URLs that could be something like https://github.com/open-telemetry/semantic-conventions.git:model

I prefer option 1 but can easily live with option 2. What do you think?

@jsuereth
Copy link
Contributor

jsuereth commented Jun 7, 2024

I agree that this initially confusing to me as well, but also with Laurent's points.

In the end, Weaver is meant to be a code generation platform. Semconv is the first (and very important) client, but the end goal is to have this available to otel users who wish to write instrumentation.

I think we need a decision here that enables both goals - (1) reduce confusion between "consume from git" vs. "consume from directory" and (2) is flexible beyond just semconv repository.

I do think we should entertain option 2 from Laurent's comments BUT I'd be happy, initially, to just improve --help for now and noodle on ways to make this less confusing that will work with other directories and git.

@lquerel
Copy link
Contributor

lquerel commented Jul 29, 2024

Now --registry, -r, and --baseline-registry support the following extended syntax: source[sub_folder].

source can be a local folder, a git repo URL, a local archive (.zip or .tar.gz), or a remote archive.
sub_folder is a folder inside the source (for git repo URLs and archives).

Examples:

  • -r /my-local-registry
  • -r https://github.com/open-telemetry/semantic-conventions/archive/refs/tags/v1.26.0.zip[model]
  • -r https://github.com/open-telemetry/semantic-conventions.git[model]

Note: --registry-git-sub-dir is now deprecated and will be removed in a future release.

@lquerel lquerel closed this as completed Jul 29, 2024
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

3 participants