microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
924 stars 287 forks source link

Batch requests between separate mgt-person components #649

Open RobPethick opened 3 years ago

RobPethick commented 3 years ago

Proposal: Improve performance by batching requests between separate mgt-person components

Description

When I put 50 mgt-person cards on the page I can see that the requests for the user and their photo are batched up (so I see 50 rather than 100 requests) but it would be great if the requests could be batched between the different components.

Rationale

This seems like a fairly common use-case and 50 API requests does put me off using this a fair bit.

Preferred Solution

Ideally this would just work out the box though I understand if this is not technically possible I don't know how possible it is for the components to communicate.

ghost commented 3 years ago

Hello RobPethick, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

shweaver-MSFT commented 3 years ago

@RobPethick, I agree that batching the Graph calls across components is a great aspiration. Today, we have one off calls to the Graph in each component. We do cluster some calls, but most components act independently. There would need to be some sort of mechanism created to manage the calls in and out of the toolkit for all components.

Back in my Silverlight days I used something aptly named the "WebRequestMaker", to manage all web requests across the app. Perhaps we could introduce a similar concept to intercept outgoing Graph calls and find opportunities to batch them.

This will definitely require some thought and prototyping. Any help is much appreciated ;)

stevebeauge commented 3 years ago

Hello,

Any progress regarding this point ?

I guess this isn't a simple issue to resovle but it forbids the usage of the toolkit for any usage that display more than 5 five people at a time.

thanks

waldekmastykarz commented 2 years ago

I wonder if this could be solved using Graph SDK middleware that could be the central point to manage requests. Recently I used this approach to see how we could avoid issuing multiple requests to the same endpoint, which is slightly related to this issue. See https://blog.mastykarz.nl/avoid-duplicate-requests-microsoft-graph-javascript-sdk/ for more info.

gavinbarron commented 11 months ago

@sebastienlevert I recall you mentioning a project that did something to automatically roll requests into batches Can you share that again please?

sebastienlevert commented 11 months ago

So @mgwojciech provided this solution for SPFx-based solution, but I think we could extend this easily to MGT. Marcin, would you be willing to help us build something like that?

mgwojciech commented 11 months ago

I think with middleware @waldekmastykarz mentioned it should not be a problem. I don't think I'll find some time within next 10 days, but if it can wait till August I'll be more than happy to help.

sebastienlevert commented 11 months ago

It's vacations and we're not expecting you to spend anytime here at all ;) We can connect back in August and we'll definitely be happy to collaborate and work together on design / implementation if you want to help us!

mgwojciech commented 11 months ago

Sounds like a plan 🙂

mgwojciech commented 10 months ago

Here is a sample: https://github.com/mgwojciech/unit-test-samples/tree/main/mgt-batching run npm install Add clientID to .env file npm start You can add more compoments if You want to test it a little bit further. In this sample I just added login, Person, Agenda, Files and Todo, and it looks like all requests are batched. I implemented batching only for get requests as Graph Batch in SDK supports only get (or at least it looks that way).

All the magic happens in https://github.com/mgwojciech/unit-test-samples/blob/main/mgt-batching/src/GraphAutoBatch.ts If You would like some more details on the implementation - don't hesitate to ask

mgwojciech commented 10 months ago

I know it's not the best quality code in the world, but I wanted to make it easy to copy-paste if needed.

sebastienlevert commented 10 months ago

Pinging @gavinbarron on this issue. I think this is a pretty interesting approach. I'm wondering how this would fit in a complex application (large apps with hundreds of calls that would need to be batched across multiple batches, for instance... Thinking about a list of 100+ records with a person component).

I'm also wondering if there are "real" performance benefits or we would just hide a lot of the complexity in a batch and debugging would be harder... But it really sounds promising if the value delivered is the right now.

Definitely I would probably make this a prop on the IProvider interface that we could set to enable this feature (or not) and it would be turned off by default.

mgwojciech commented 10 months ago

I have this feeling there is a way to do this with middleware. If that would be the case it would be more elegant and easier to integrate in mgt itself. I will keep this in my background thread this week and try to push it next week (if I come up with something). When it comes to multiple records - I did some testing and it looks like there is something in the component that slows it down. Like it waits for data for all components of one type to render all at the same time. I'll investigate further :)

sebastienlevert commented 10 months ago

Thanks @mgwojciech! This is really appreciated! Happy to brainstorm in the next weeks on this approach!

gavinbarron commented 10 months ago

it's an interesting approach, there's a bit of a downside in that we might lose component level telemetry, it just depends on how the request headers for each underlying request in the batch are unpacked and processed by the telemetry pipeline.

I think that the middleware idea is really interesting and potentially more flexible, as then auto batching could be leveraged by any customer using the JavaScript SDK.

Awesome stuff @mgwojciech!

carlo-quinonez commented 3 weeks ago

Any updates on this? We just encountered the same problem.

mgwojciech commented 3 weeks ago

Hi @carlo-quinonez

If I remember correctly, the issue is that mgt-persona batches user, presence and photo per user and it's impossible to batch a batch.

I ended up with implementing my own batch library and persona card. If You feel like it, find the library here: https://www.npmjs.com/package/mgwdev-m365-helpers

Here You can find a sample implementation using Fluent9 https://github.com/mgwojciech/MGWDev.SPEmbedded/blob/master/client-app/src/components/GraphPersona.tsx This particular sample is using react global context, but You can easily replace it with passing graph client as component property if that makes more sense in Your case.

In general, I don't think it's feasible to actually fix that in mgt, as I believe it would force quite a refactoring.

As a bonus, take a look at beta profile endpoint, can be quite useful for extended properties: https://learn.microsoft.com/en-us/graph/api/profile-get?view=graph-rest-beta&tabs=http