router
router copied to clipboard
apollographql
Wrap `UsageReporting` in `Arc` to avoid expensive clone
Cloning the UsageReporting structure to use in extensions (https://github.com/apollographql/router/blob/dev/apollo-router/src/query_planner/caching_query_planner.rs#L390) has a fairly higher cost that I would expect. I didn't realize it held a map of referenced fields which can grow large (https://github.com/apollographql/federation-rs/blob/35052d8064e7ca50d09226febdbbe0d55cc5d18b/federation-2/router-bridge/src/planner.rs#L313)
The change has less impact than I thought on the rest of the code, sounds like it may be worth it to wrap into an Arc. Thoughts?
Looks like extensions need to be changed (get::<UsageReporting>()) Not sure how to proceed without breaking changes 😢
looks like the CI is not running, is it because the branch is on your repository?
looks like the CI is not running, is it because the branch is on your repository?
Yeah no idea 🤔
@Geal this is a breaking change to the type map. Not sure if we can go ahead with this unfortunately. Maybe bundle that with the Arc<Query> one in the next major version?
UsageReporting is not exposed in the public API of the router, so it's fine. Anything in the context extensions that uses a private type will only be accessible from inside the router's code
The problem is the type is public I believe: https://github.com/apollographql/federation-rs/blob/35052d8064e7ca50d09226febdbbe0d55cc5d18b/federation-2/router-bridge/src/planner.rs#L306
I don't think there's a private wrapper for it in the router?
the type is public in router-bridge, but the apollo-router crate does not reexport it: https://docs.rs/apollo-router/latest/apollo_router/?search=usagereporting
huh TIL, didn't know it had to be reexported as part of router. Thought the type ID just had to match. Thanks @Geal
technically, if someone uses the apollo-router crate and imports router-bridge manually too, then yes they would be able to access that context key. But I'd be willing to disregard that as a breaking change
technically, if someone uses the apollo-router crate and imports router-bridge manually too, then yes they would be able to access that context key. But I'd be willing to disregard that as a breaking change
Ohhh I see, that was the confusion, we're on the same page then 👍
@xuorig We have our repo setup to run on forked repos, so I'm not sure why it won't run. Our CircleCI logs are, for whatever reason, telling US, to tell you to log out and log back in to CircleCI. I do wonder if this has something to do with both of our organizations having SSO setups or something, but to see how this goes, I've temporarily adopted your PR and pushed it to https://github.com/apollographql/router/tree/abernix/xuorig/arc-usage-reporting which has kicked off this CircleCI run, if you want to watch it. I did have to merge in our dev trunk, but it's running now. If goes through, I'll open a PR and be sure to give you commit credit.
Signed out and signed back in of my circle ci account, let's see 👀 seems to be working?
(Oh, don't worry about the Netlify Deploy preview. That definitely won't work for you because we don't pay for it to work.)
Closing this as it was merged in https://github.com/apollographql/router/pull/4752