Filter Extension Re-organisation

[XAMPPRocky]

This Pr builds on-top of #286 and refactors the API structure of the filter changes to be easier to use, and cleans up the public API surface.

Changes:

• All filter extension modules are now public.
• All filters and filter factories are removed from public API. Previously we exposed and asked the user to create the factory type themselves. However we don't actually need to expose the concrete types since we are only ever using them as trait objects. Hiding these implementation details will give us more freedom in the future to change and optimise the layouts of filter and filter factory structs without worrying about those details being relied on by users.
• To replace the public factory types, every filter extension module as a convention now has a top level free function called factory that constructs a new instance of DynFilterFactory. This makes paths to construct filters much less verbose while conveying the same information, and hiding the underlying implementation. Considering the following example.

  // Old
  let factory = quilkin::filters::extensions::DebugFactory::new(&logger);
  // New
  let factory = quilkin::filters::debug::factory(&logger);

• The filter extension documentation that was external has been moved into respective module's API documentation. This felt like it made more sense, as this would allow this documentation to use intra-doc links, and also has the side benefit of removing the nightly dependency for testing. Using rustdoc here reduces the amount of boilerplate we need to document and removes a lot of potential for docs to get out of sync, as we can document things like configuration through the public Rust types, even if you aren't using Rust, you'll be able to see what you can set.
• The extensions module is removed, and some filter extensions were further broken up into submodules if they had a significantly enough complex configuration type and traits.

closes #280

Filter Extension Docs Example

[iffyio]

This sounds good to me and thanks for the detailed PR description. I'm okay with bringing the docs in the code, been pondering trying it out to get rid of the # rust code we have in our yamls when viewing outside rustdoc/mdbook and glad to see it makes sense!
One part I'm hoping works but not sure of is whether we can link to a particular section in another page/module-doc? e.g being able to link to the ### metrics section of filter A's documentation from filter B's documentation? I'm hoping its possible since its all markdown but not too sure?
If so this looks good to me and (not for this PR) I think we should also bring in the other docs pages like xds, session, proxy etc too where they make sense so we don't have documentation split between two places.
Curious what @markmandel thinks of this approach overall

[markmandel]

Curious what @markmandel thinks of this approach overall

Yeah - love it! Moving stuff into rustdoc is great - I figured we would end up there eventually, but likely after release, just because we're in a bit of a limbo at the moment, and it's hard to track and link between both, since we don't have a release in cargo yet.

The only downside I see: It's harder to keep improving documentation between releases. If we want to update docs for something that's hosted on https://docs.rs/ we have to cut a whole new release.

But we can likely be smart about what we move and what we don't. Reference docs are probably better in rustdoc, whereas guides / tutorials might live better in Github (which is probably the things we'll want to improve incrementally).

Thoughts?

[XAMPPRocky]

The only downside I see: It's harder to keep improving documentation between releases. If we want to update docs for something that's hosted on https://docs.rs/ we have to cut a whole new release.

But we can likely be smart about what we move and what we don't. Reference docs are probably better in rustdoc, whereas guides / tutorials might live better in Github (which is probably the things we'll want to improve incrementally).

Thoughts?

FWIW, the convention in most Rust projects is to use https://github.com/rust-lang/mdbook for guide level information. As it’s a lot more powerful than GitHub rendered markdown, and it’s pretty easy to deploy to GitHub Pages.

• Examples: cargo, rust-gpu, mdbook itself

Along those same lines, if we want to have rendered API documentation available for the latest main, it’s pretty simple to run cargo doc and deploy that output to GitHub Pages.

[iffyio]

The only downside I see: It's harder to keep improving documentation between releases. If we want to update docs for something that's hosted on https://docs.rs/ we have to cut a whole new release.

Right this makes sense! having guide/tutorial types docs in something like mdbook and reference as rust docs sounds good to me!

[XAMPPRocky]

Alright, I've addressed all the merge conflicts, so this should be in right state for review. I've also added one additional change which is the removal of the quilkin::filter proc macro. The new pattern was incompatible with having the ID exposed on the Filter type, so it didn't really feel like it was providing a benefit, now the names are just consts in the module. I have a better idea for what quilkin::filter proc macro could look like that'll make a separate issue about.

[markmandel]

The only downside I see: It's harder to keep improving documentation between releases. If we want to update docs for something that's hosted on https://docs.rs/ we have to cut a whole new release.

Right this makes sense! having guide/tutorial types docs in something like mdbook and reference as rust docs sounds good to me!

Sounds good to me! Let's do it.

[quilkin-bot]

Build Succeeded 🥳

Build Id: 70006958-9c15-4144-b5b1-086f89da0b81

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/293/head:pr_293 && git checkout pr_293
cargo build

[markmandel]

Just had a look through - this is a much cleaner implementation surface 👍🏻 love it.

I'm being cognisant of making sure we stay in a state of "release ready" - just because I know releases can turn around pretty fast, once all the approvals line up.

I'm thinking we should we add a todo block, or a broken link to where we think the docs.rs documentation should go in the README.md for now, particularly around Filter configuration - just so we're prepared for when things actually release, and also don't end up losing track of the documentation.

Other than that, this looks great. We should probably write a ticket for moving the other docs to mdbook as well. I particularly love that we won't need nightly to test out the inline documentation 🥳 and can get rid of that as dev tooling entirely.

So it's really just documentation stuff that blocks me from approving this (I'm all about that developer experience 😄). @iffyio anything on your end?

[quilkin-bot]

Build Succeeded 🥳

Build Id: cf79d888-a065-46e3-8563-fcee5447cb06

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/293/head:pr_293 && git checkout pr_293
cargo build

[markmandel]

Alright, I've pulled out all the documentation changes for now, and I'll make those in separate PR.

Thanks so much 🙌🏻 - this is much easier to review now. Once we land #319 that will also likely resolve much of my concerns, as we can host our own documentation on our gh-pages.

There are some changes here to existing docs that I'd like to do once we land #319, since then we'll have the version snapshot (or we could link do the release branch docs????) and won't be break the experience for new users.

Focusing on release atm, but will jump on this once that's done, because I really like this new API surface.

[quilkin-bot]

Build Succeeded 🥳

Build Id: 60cce3e2-677f-4055-9018-2fe8af24ce59

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/293/head:pr_293 && git checkout pr_293
cargo build

[markmandel]

I was hoping that #319 's snapshotting would solve this issue, but looks like that's blocked for the moment.

Since this is a change in API, and this will break the docs for 0.1.0 -- I'm wondering how to handle this in the short term.

Maybe we add a note here:

That says something like:

This documentation is for the development version of Quilkin. To view the documentation for a specific release, switch to the release tag in the above dropdown.

(I'm happy to do this as a seperate PR if we are all happy with it).

Which can then we can remove it once #319 is merged and good to go.

Other than that, this is 👍🏻 as far as I am concerned - and I want to get it in, as I know a lot of your follow up work is based on this.

Sound good?

[XAMPPRocky]

SGTM, I can make a separate PR.

[quilkin-bot]

Build Succeeded 🥳

Build Id: 862f1be6-1727-462b-a86c-5eca7d83bc6a

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/293/head:pr_293 && git checkout pr_293
cargo build