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

Generate attributes from files in symlinked directories #441

Open
awangc opened this issue Nov 4, 2024 · 3 comments · May be fixed by #468
Open

Generate attributes from files in symlinked directories #441

awangc opened this issue Nov 4, 2024 · 3 comments · May be fixed by #468
Labels
contribfest enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@awangc
Copy link

awangc commented Nov 4, 2024

Is your feature request related to a problem? Please describe.
When trying to run weaver registry generate on a local path, I'd like the ability to "compose" the model/ folder via symlinking directories present in the filesystem. Currently if I do symbolic linking (ln -s) weaver does not read from the linked directories when generating the attributes.

Describe the solution you'd like
An option that can be passed to weaver that would make it read sym linked folders under model/ and generate attributes

Describe alternatives you've considered
Right now the only way to achieve what I'm thinking is if I copy specific directories into the model/ folder. This means if I want to keep an in-house component that need to be coalesced with semantic conventions then changes from either side need be explicitly propagated to the model/ folder, which seems a little more cumbersome than allowing changes to be propagated via symbolic links.

@lquerel lquerel added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Nov 5, 2024
@Leo6Leo
Copy link

Leo6Leo commented Nov 20, 2024

Hello @lquerel and @jsuereth , I would like to work on this issue!

Here is my approach after some play around and observation:

  1. Modify the functions in Impl RegistryRepo to respect symlink options when walking directories
  2. Add a new bool follow_symlinks flag to RegistryGenerateArgs in the CLI, and pass the value of the flag follow_symlinks into the registry_repo variable.
  3. Add appropriate tests to verify the behavior

The ideal outcome would be:
After running the command

weaver registry generate -r ./semantic-conventions/model -t ./semantic-conventions/templates markdown -follow_symlinks=true

weaver will read symbolic linked folders under sementic-conventions/model and generate the artifacts.

And my question would be:

  1. Is this approach on the right track?
  2. Do we need to modify the FileSystemLoader too? (i.e supporting the symbolink for template files)

PS: new to OTel community, Let me know your thoughts on above. Thanks!!

@lquerel
Copy link
Contributor

lquerel commented Nov 20, 2024

Hello @Leo6Leo, To start, it's really cool that you’re helping out with Weaver!
To start, it’s really great that you’re lending a hand with Weaver!

Regarding the approach you described, RegistryRepo doesn’t handle directory traversal. Instead, it focuses on creating a local repository in the case of a remote registry (git repo or archive). To modify the code for directory traversal, you’ll need to work on the resolution part in SchemaResolver::load_semconv_from_local_path (everything is a local directory for the resolver). You should add a follow-symlinks parameter as you proposed and use the follow_links method of WalkDir. This also means propagating this parameter through the entire upstream call chain.

Then, at a minimum, you’ll need to update the following commands: registry check, registry resolve, and registry generate.

I also recommend defining the follow_symlinks field in a structure annotated with Clap that will be shared among these commands using the #[command(flatten)] annotation.

Let me know if you need more guidance on this! Thanks

@Leo6Leo Leo6Leo linked a pull request Nov 20, 2024 that will close this issue
@Leo6Leo
Copy link

Leo6Leo commented Nov 20, 2024

@lquerel Thank you for the warm welcome and quick response! I have created a PR to fix this.

#468

Since I am new to Rust and OTel, I am open to suggestions and feedback. Again thanks for spending time providing me with guidance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribfest enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants