Skip to content

Conversation

syphar
Copy link
Member

@syphar syphar commented Oct 15, 2025

This is a bigger refactor and I have to apologize in advance for the size of it. I have ~8 WIP branches where I tried to do only parts of the rewrite, but all failed and now I ended up doing many things at once.

The initial motivation of this is coming from wanting to make progress on the sync/async migration in our codebase. Plenty of things were and still are not in the state I want them to be.

For some parts I took inspiration from the crates.io codebase and how they do it, which I like.

To wrap it up, this PR:

  • updates the Config struct to include a builder.
  • changes the Context trait to a struct that just includes everything. This made much of the migration much easier, less lifetime errors.
  • the places in the codebase that previously worked with a tokio::runtime::Runtime directly, now work with a tokio::runtime::Handle. This enables us to use tokio::test and also later migrate our main function to tokio::main.
  • instead of the old test-wrappers, you can now just create a TestEnvironment in your test, and the cleanup happens in the Drop impl.

some detail notes about the implementation:

  • I also tried bon because I like its design more, and crates.io uses it too, but since I wanted to "stack" the builder calls it was too annoying to work with.
  • test diff is mostly: changes how to access the context content, or just whitespace / indendation, when I replaced the wrapper usage with the "new way".
  • I only updated the tests that used override_config for now, some some selected ones. The rest follows later.

known limitations for now:

  • the context is not lazy any more, which might slow down some tests, where test-setup is done that we might not need in the test
  • default tokio::test doesn't work, it only works with flavor = multi_thread. This is because I want to use block_in_place to be able to teardown the test env in Drop. I took this from crates.io.

after this PR I imagine:

  • renaming some methods on the context
  • rewriting the other tests using the standard async_wrapper

@syphar syphar self-assigned this Oct 15, 2025
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Oct 15, 2025
@GuillaumeGomez
Copy link
Member

We should also start adding some benchmarks as I think it's becoming more and more important.

@syphar syphar force-pushed the config-builder-derive branch from 3e2baf7 to 2fe1d24 Compare October 15, 2025 18:05
@syphar syphar marked this pull request as ready for review October 15, 2025 18:05
@syphar syphar requested a review from a team as a code owner October 15, 2025 18:05
@syphar syphar force-pushed the config-builder-derive branch from 2fe1d24 to 0679ed7 Compare October 15, 2025 18:07
@syphar
Copy link
Member Author

syphar commented Oct 15, 2025

We should also start adding some benchmarks as I think it's becoming more and more important.

for which parts?

in my mind, most of the time is still spent in the S3 fetch, so low-level speed doesn't really matter that much.

My focus is more around code-quality, readability, ...

( also I want to work on the subdomain/XSS part when it's easier to add :) )

@syphar syphar force-pushed the config-builder-derive branch from 0679ed7 to 2c5c6ca Compare October 15, 2025 18:10
@GuillaumeGomez
Copy link
Member

All parts I guess? That would allow us to keep track of which part is taking how much time. Not sure how important it is though.

in my mind, most of the time is still spent in the S3 fetch, so low-level speed doesn't really matter that much.

Yeah I guess too. But having actual numbers to see if our changes start to take a bigger % would be useful on the long run.

My focus is more around code-quality, readability, ...

On that we both agree. Code quality comes before performance as long as performance is not problematic (and performance is not a problem currently).

( also I want to work on the subdomain/XSS part when it's easier to add :) )

Oh nice. :)

@syphar
Copy link
Member Author

syphar commented Oct 15, 2025

in my mind, most of the time is still spent in the S3 fetch, so low-level speed doesn't really matter that much.

Yeah I guess too. But having actual numbers to see if our changes start to take a bigger % would be useful on the long run.

we already see performance tracing numbers in sentry for that.

@syphar syphar force-pushed the config-builder-derive branch from 2c5c6ca to b4a5fad Compare October 15, 2025 18:47
@GuillaumeGomez
Copy link
Member

It gives details on what took time in a query or just tells how much time it took?

@syphar syphar force-pushed the config-builder-derive branch from b4a5fad to d4f302d Compare October 17, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants