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

chore(external docs): generate global option configuration automatically from Rust code #22345

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

titaneric
Copy link
Contributor

Summary

Aiming to solve the issue mentioned in #21348 (comment), and resolve it by generate a global option configuration page by introducing a new global_option configurational component macro. I also update the corresponding Ruby code for generating component docs. It looks worked.

I want to emphasize that this is not a production code but rather that a PoC. I would refactor the code (especially for the Ruby code, first time to write Ruby) until all reviewers would agree my solution.

I guess it would be a very long procedure to review the code and the generating documents, especially for the difference the manually-written configuration page and the generated one from Rust code. I will help them as much as possible.

TODO:

  1. get every reviwers' approvals for the change
  2. introduce global_option configurational component macro for EVERY global option configuration
  3. introduce overlay and decorated page for the configuration page like to one in website/cue/reference/components/transforms/log_to_metric.cue
  4. write dev docs if needed

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

make generate-component-docs
cd website
make run-production-site-locally

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
    • make check-all is a good command to run locally. This check is
      defined here. Some of these
      checks might not be relevant to your PR. For Rust changes, at the very least you should run:
      • cargo fmt --all
      • cargo clippy --workspace --all-targets -- -D warnings
      • cargo nextest run --workspace (alternatively, you can run cargo test --all)
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

Enrichment table
image

Secret
image

@github-actions github-actions bot added domain: ci Anything related to Vector's CI environment domain: external docs Anything related to Vector's external, public documentation labels Feb 2, 2025
Copy link
Member

@pront pront left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @titaneric! This is a reasonable approach and should eliminate a lot of confusion in this area.

@@ -1728,6 +1728,42 @@ def render_and_import_component_schema(root_schema, schema_name, component_type,
)
end

def render_and_import_global_option_schema(unwrapped_resolved_schema, friendly_name, config_map_path, cue_relative_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tangential: #22350

@@ -111,6 +112,9 @@ pub struct FileConfig {
/// [rfc3339]: https://tools.ietf.org/html/rfc3339
/// [chrono_fmt]: https://docs.rs/chrono/latest/chrono/format/strftime/index.html#specifiers
#[serde(default)]
#[configurable(metadata(
docs::additional_props_description = "represent mapped log field names and types."
Copy link
Member

@pront pront Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
docs::additional_props_description = "represent mapped log field names and types."
docs::additional_props_description = "Represents mapped log field names and types."

We would also benefit from an example here.

@@ -18,10 +18,11 @@ pub mod geoip;
pub mod mmdb;

/// Configurable enrichment tables.
#[configurable_component]
#[configurable_component(global_option("enrichment_tables", "enrichment table type"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding, the second string is a description which can be omitted right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right!
I remove it and re-generate the component docs, and there is no diff at all.

I will remove it.

@pront pront marked this pull request as ready for review February 3, 2025 14:26
@pront pront requested review from a team as code owners February 3, 2025 14:26
@titaneric
Copy link
Contributor Author

titaneric commented Feb 3, 2025

Hi @pront , I just want to know that should I continue to work for the listed TODO?

I mean updating ALL of the global option with wrap configurable component macro and Ruby code refactor.

I could try incremental change (multiple PRs) to update the support the global configuration to this new form.

@pront
Copy link
Member

pront commented Feb 3, 2025

Hi @pront , I just want to know that should I continue to work for the listed TODO?

I mean updating ALL of the global option with wrap configurable component macro

Ah didn't realize we have more places to update. Can you describe a bit more?

and Ruby code refactor.
Yes on the Ruby refactor. (just to be clear the Rust issue is orthogonal you can ignore that)

I could try incremental change (multiple PRs) to update the support the global configuration to this new form.

Up to you 👍

@titaneric
Copy link
Contributor Author

Ah didn't realize we have more places to update. Can you describe a bit more?

I only update the secret and enrichmentable with the new configurable macro, but there are more options such as data_dir, api and so on.

I might introduce the base and overlay global option configuration just like other components do. I will work on it!

@pront
Copy link
Member

pront commented Feb 3, 2025

Ah didn't realize we have more places to update. Can you describe a bit more?

I only update the secret and enrichmentable with the new configurable macro, but there are more options such as data_dir, api and so on.

I might introduce the base and overlay global option configuration just like other components do. I will work on it!

Sounds great. Also, cc @jszwedko (just in case you want to take a look)

@titaneric
Copy link
Contributor Author

I might need couple days to work on it. Should I convert to draft and then turn into ready once I finished, or stay current state and comment it in this thread?

@pront
Copy link
Member

pront commented Feb 3, 2025

I might need couple days to work on it. Should I convert to draft and then turn into ready once I finished, or stay current state and comment it in this thread?

You can leave it as is. Take as as much time you need and thank you again. Much appreciated.

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be the first external contribution modifying the docs generation components ❤️ Thanks for diving into this @titaneric

@pront pront added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Feb 4, 2025
@titaneric
Copy link
Contributor Author

titaneric commented Feb 5, 2025

@pront @jszwedko @buraizu , I think I have finished the change. Please review!

Please use difft to review enrichment_table and secret schema change.

git checkout master
./scripts/cue.sh export -e configuration.configuration.enrichment_tables > before-enrichment_tables.json

git checkout feat/global-options-auto-docs
./scripts/cue.sh export -e configuration.configuration.enrichment_tables > after-enrichment_tables.json

difft ./before-enrichment_tables.json ./after-enrichment_tables.json

before-enrichment_tables.json
after-enrichment_tables.json

git checkout master

# note the single form of `secret` instead of `secrets`
./scripts/cue.sh export -e configuration.configuration.secret > before-secrets.json

git checkout feat/global-options-auto-docs
./scripts/cue.sh export -e configuration.configuration.secrets > after-secrets.json

difft ./before-secrets.json ./after-secrets.json

before-secrets.json

after-secrets.json

@@ -76,7 +77,7 @@ pub struct FileConfig {
/// 1. One of the built-in-formats listed in the `Timestamp Formats` table below.
/// 2. The [time format specifiers][chrono_fmt] from Rust’s `chrono` library.
///
/// ### Types
/// Types
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the header here since it would be displayed on the right side

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, yeah it might better without that header.

#[allow(clippy::large_enum_variant)]
#[configurable_component]
#[configurable_component(global_option("secrets"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that stood out here is that the old docs define secret: https://vector.dev/docs/reference/configuration/global-options/#secret

And the new docs secrets with a s. Although the latter is better, it seems like a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree it's indeed a breaking change for documents.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we revert it to secret? If I understand correctly, it's just a rename. But if not, we can mark this as a breaking change (fix is trivial)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try it, wait a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right!
I SHOULD make it consistent to config instead of folder name.

I have renamed it.

}
}
}
// TODO: generate `common` and `required` fields from ruby according to some tags
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these TODOs something you plan to address in this PR? (it's fine to leave as is, someone can follow up later in another PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I have some basic thought for how to solve these TODO by introducing a new configurable macro for global options only. I will start to work on it in this weekend.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇 No rush, and we can make it a followup PR

@pront pront enabled auto-merge February 5, 2025 18:05
auto-merge was automatically disabled February 6, 2025 01:27

Head branch was pushed to by a user without write access

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: ci Anything related to Vector's CI environment domain: external docs Anything related to Vector's external, public documentation no-changelog Changes in this PR do not need user-facing explanations in the release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants