microsoftgraph / msgraph-sdk-go

Microsoft Graph SDK for Go
https://docs.microsoft.com/en-us/graph/sdks/sdks-overview
MIT License
232 stars 34 forks source link

Issue with the size of the API surface of the models package #129

Closed breml closed 1 year ago

breml commented 2 years ago

The way, this Go module and especially the models package are constructed, leads to severe issues with the go tooling (and linting).

We have a simple Go application where we added the functionality to send email notifications though the Microsoft Graph API using Mail.Send. So we added the necessary packages, namely:

import (
    "github.com/Azure/azure-sdk-for-go/sdk/azidentity"
    authentication "github.com/microsoft/kiota-authentication-azure-go"
    msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go"
    "github.com/microsoftgraph/msgraph-sdk-go/me/sendmail"
    "github.com/microsoftgraph/msgraph-sdk-go/models"
)

for this purpose.

The final code did what was expected (sending the mail), but it also made the duration for go test to jump from 7s to 8m 50s and the linting with golangci-lint to jump from 36s to 7m 13s in our Github Action pipeline.

We then changed the Graph API request to a "plain HTTP Post request" using the net/http package from the standard library (while still using github.com/Azure/azure-sdk-for-go/sdk/azidentity and github.com/microsoft/kiota-authentication-azure-go for the authentication) and we are back at the normal times for tests and linting.

Additional pointers for the excessive size of the github.com/microsoftgraph/msgraph-sdk-go/models package are the official documentation refuses to display the API because it is too large and also Github only shows the first 1000 files when listing the content of said package folder.

So in my opinion, the API surface of the package github.com/microsoftgraph/msgraph-sdk-go/models is way to broad and this should be refactored massively.

jamesjmurtagh commented 1 year ago

Hi everyone, We've just released v0.41.0 which takes advantage of generics and significantly reduces the amount of code being generated (see #284) As always, we're eager to see how your experience has changes and how much of an improvement you're measuring.

On top of that we have another change which will most likely become available by next week and reduces the number of methods we're emitting.

If the performance is still not good enough at that point, we'll explore:

Thanks for the patience and continuous feedback!

Thanks for the efforts you're all putting in to resolving this issue. My project still takes an abnormally long time to compile even after upgrading to v0.41.0; is there a rough ETA/timeline for the other 2 issues that you mentioned will be explored?

baywet commented 1 year ago

We don't have strict ETAs at this moment to share, but they are high priority items on the list. So, we're talking weeks rather than months. Would you mind sharing numbers (how long before 0.41, with 0.41, what would be acceptable) ?

gvanriper commented 1 year ago

I am commenting to also point out that I am now having horrible build/test times due to the msgraph-sdk-go package. This greatly affects our ability to develop especially running in Docker.

Before 0.41: 15 minutes to build from Docker (918s) After 0.41: 7 minutes to build from Docker (457s)

Removing msgraph-sdk-go in the same project: 21 seconds to build from Docker.

Please figure out a way to not provide so much bloat to this package. The frustrating part on my end is I am only using this for one tiny piece of msgraph.

jamesjmurtagh commented 1 year ago

I don't have exact figures available but within my dev environment adding this package takes build times from < 30 seconds up to >5 minutes. On top of that, these long build times get in the way of standard Go tooling for VSCode from providing 'real time' warnings/errors feedback which makes the entire dev experience really chunky and frustrating.

baywet commented 1 year ago

Thanks everyone for the additional information. @gvanriper what was the version you were using before 0.41.0? We're also working on improvements to the self-serve experience so it's easier to get a client with just the endpoints you care about, I'll share more about this here when we have something new to try.

gvanriper commented 1 year ago

@baywet , I was on v0.39.0 before.

Yes, being able to target only the resources that you need for the package I think would greatly help. I appreciate you taking the time to getting this cleared up.

If you need any other additional info, please let me know.

mblaschke commented 1 year ago

With one of the last versions the situation got worse again. It even crashes a simple go build inside Docker for Desktop because of the huge memory usage when building models (according to the kill message)

go get -u is also back at ~60 minutes.

baywet commented 1 year ago

Thanks for sharing all that information with us. We've also observed similar behavior where using generics has improved performance at the cost of increased memory usage. That's ok on most local dev machines (usually lots of memory available) but it's problematic for CI (usually agents have memory constraints). We're reviewing what can be done (better use of generics? rollback? support from the Go community?) and we'll share progress when we can. In the meantime, we're also doing the following:

mblaschke commented 1 year ago

Sorry, I have to disagree with the "that's ok on most local dev machines" statement. My colleagues had stopped working with msgraph-sdk-go as it's killing their notebooks. So far only MacBooks with i9 (runs sometimes in thermal throttling) and M1 are powerful enough to keep decent working speed. Unfortunately not every developer gets powerful machines and good notebooks have a huge delivery time currently 😞

As workaround it seems that GOMAXPROCS=200 go get -u is solving the go get -u problem (60 min -> ~10 sec?!). This is really strange as there is no real cpu or even I/O load when running go get -u 🤔

baywet commented 1 year ago

Agreed, I wasn't precise enough in my prior statement.

Thanks for the additional information, that awfully sounds like CPU trashing (something we often see in Java with memory constrained environments). We're looking into it.

Andrei-Predoiu commented 1 year ago

Hi, if i may make a suggestion.

Why not generate the following files in their own folder and package. This way we can import and download them to generate a client without importing the whole sdk

msgraph-sdk-go@v0.44.0
graph_base_service_client.go
graph_request_adapter.go
graph_service_client.go

Most of the time devs only need to import a few packages that they are working with Fx: github.com/microsoftgraph/msgraph-sdk-go/users. Unfortunately, to create a graph client, we have to import github.com/microsoftgraph/msgraph-sdk-go which will download the whole sdk Golang, afterwards, during build/install, has to work extra hard to remove the unnecessary symbols from the binary.

This one change would mean big improvements to most of the build time related issues.

tomasmota commented 1 year ago

I have to say that this issue makes the package nearly unsusable, not sure how fixing this is not the top priority. If I had any alternative at all, I would stay away from this package because of this issue.

baywet commented 1 year ago

Thanks for the feedback. This is being actively investigated for the past month. We don't have results to share just yet however.

bijujo1 commented 1 year ago

I have got a Go program deployed as a Google Cloud function. Recently I added a functionality that uses msgraph-sdk-go. Once I added it to import list, I can't even deploy the function in GCP. Deployments error out after trying for ~25 minutes. Once I remove msgraph-sdk-go and its references, deployment finishes in ~3 minutes.

friedrichg commented 1 year ago

Using go version go1.18.5 darwin/amd64

Added

import (
       "github.com/microsoftgraph/msgraph-sdk-go"
       "github.com/microsoftgraph/msgraph-sdk-go/applications"
       "github.com/microsoftgraph/msgraph-sdk-go/applications/item/addpassword"
       "github.com/microsoftgraph/msgraph-sdk-go/models"
)

that included

github.com/microsoftgraph/msgraph-sdk-go v0.46.0

in the go.mod

Before the change

$ time go install -mod=mod -v ./cmd/tool

real    0m5.507s
user    0m7.689s
sys 0m3.194s

After the change

$ time go install -mod=mod -v ./cmd/tool
...
real    0m41.992s
user    0m21.404s
sys 0m18.307s

If we remove the cache:

Before the change

$ go clean -cache -modcache
$ time go install -mod=mod -v ./cmd/tool
...
real    1m14.897s
user    4m53.222s
sys 0m54.485s

After the change

$ go clean -cache -modcache
$ time go install -mod=mod -v ./cmd/tool
...
real    12m12.315s
user    32m36.017s
sys 9m3.591s

Other microsoft/azure libraries are already present in go.mod and don't have this issue

        github.com/Azure/azure-sdk-for-go v63.0.0+incompatible // indirect
        github.com/Azure/go-autorest v14.2.0+incompatible // indirect
        github.com/Azure/go-autorest/autorest v0.11.25 // indirect
        github.com/Azure/go-autorest/autorest/adal v0.9.18 // indirect
        github.com/Azure/go-autorest/autorest/date v0.3.0 // indirect
        github.com/Azure/go-autorest/autorest/to v0.4.0 // indirect
        github.com/Azure/go-autorest/autorest/validation v0.3.1 // indirect
        github.com/Azure/go-autorest/logger v0.2.1 // indirect
        github.com/Azure/go-autorest/tracing v0.6.0 // indirect
baywet commented 1 year ago

@friedrichg thanks for sharing those numbers. For clarification, which changes are you referring to?

friedrichg commented 1 year ago

I am referring to a go project in which I just included this library( I added the imports), the implementation is done, we are calling 3 functions in msgraph and that is correctly functioning. But the PR is taking so much memory, cpu and time that we are thinking already of alternative implementations.

tomasmota commented 1 year ago

Any news on this @baywet ? Think I'll have to fork this and remove 99% of the models until it gets fixed.

baywet commented 1 year ago

We're days (maybe a week?) away from having a new version that significantly improves built times. Here are some of the results of our investigations:

If you don't want to wait until then/would like to experiment with something, I have a better alternative for you than forking this repo which will allow you to generate a client for the API surface you care about, with the same development experience.

  1. go to https://app.kiota.dev
  2. search for "graph"
  3. click "show" on the last one (github::microsoftgraph/msgraph-metadata/graph.microsoft.com/v1.0)
  4. build your include and exclude minimatch patterns to filter the API down to the paths you only care about (note, all paths start with /)
  5. Once you're happy with the tree view, click "generate"
  6. Make sure to select the right language, and update the class name as well as the namespace prefix (project/dir/dir/dir)
  7. Click Generate

Keep in mind this is all running in the browser and an experiment at this point. It'll be slow to generate, your browser might tell you it's unresponsive (but it's doing the work). All of that is due to how large is the Microsoft Graph description. If you think it's too slow, you can always use the CLI instead which is in public preview. Don't hesitate to share your experience!

Andrei-Predoiu commented 1 year ago

Thanks for the great news. Looking forward to using the new graph setup.

jackhopner commented 1 year ago

Thanks @baywet appreciate that little web app you've made!

Just moved over to a generated client using it, for anyone looking to use it and wanting to find the api paths to include/exclude I found the graph explorer very useful (especially for the delta endpoints as they paths aren't what you'd expect).

Had to manually copy over the graph request adapter as it wasn't in the generated code (maybe I configured badly).

https://developer.microsoft.com/en-us/graph/graph-explorer

Overall my build time went from ~16mins -> 3.5mins which is life changing for day to day development!

baywet commented 1 year ago

Thanks for the feedback!

What if you could generate a client straight from graph explorer by just selecting the paths you want and click "get a client"?

Also, with that self served client, how is the auto complete experience?

tomasmota commented 1 year ago

Sorry to advertise other projects here, but for anyone who might be looking for an alternative, I just found this when looking at the terraform azuread provider: https://github.com/manicminer/hamilton. Managed to get it working in 2 minutes by copying the example, no crazy build times with that.

jackhopner commented 1 year ago

Thanks for the feedback!

What if you could generate a client straight from graph explorer by just selecting the paths you want and click "get a client"?

Also, with that self served client, how is the auto complete experience?

That would definitely be a smoother experience.

Autcomplete is smooth for me as I'm only using a few functions from the graph client.

baywet commented 1 year ago

Hi everyone 👋, I do have great news to share! We've just released v0.48.0 (and 0.49.0 for beta). Out internal tests show it drastically reduces both build time and memory usage (4 minutes instead of 17 for v1, 9 minutes instead of 48 for beta). It also drastically improves packages caching after the initial build. But of course, we want to hear from you!

What changed? We've flattened most of the directory structure for the fluent API to the first level folder (/me/drive/driveitem is now under /me).

What are the breaking changes when upgrading?

Example

import (
-    "github.com/microsoftgraph/msgraph-sdk-go/users/item"
+   "github.com/microsoftgraph/msgraph-sdk-go/users"
)

-requestConfiguration := &item.UserItemRequestBuilderGetRequestConfiguration {
+requestConfiguration := &users.UsersUserItemRequestBuilderGetRequestConfiguration {
-   QueryParameters: &item.UserItemRequestBuilderGetQueryParameters {
+   QueryParameters: &users.UsersUserItemRequestBuilderGetQueryParameters { 
      Select: string[] {"displayName"}
   }
}

result, err := client.UsersById("jane@contoso.com").Get(context.Background(), requestConfiguration)
bartek commented 1 year ago

Thank you for this! We're seeing some nice improvements in timing, especially when the build is cached.

v0.48.0:

❯ time go build -a
go build -a  258.95s user 53.51s system 329% cpu 1:34.83 total
❯ time go build
go build  6.86s user 1.66s system 118% cpu 7.184 total
❯ time go build
go build  0.97s user 0.90s system 347% cpu 0.539 total

Previously v0.44.0:

❯ time go build -a
go build -a  707.41s user 167.62s system 601% cpu 2:25.59 total
❯ time go build
go build  10.87s user 6.87s system 93% cpu 18.901 total
❯ time go build
go build  2.95s user 3.34s system 232% cpu 2.712 total
sabre1041 commented 1 year ago

I too can confirm a significant reduction in build time. Great work by the team on the recent version!

mblaschke commented 1 year ago

Can confirm too, thanks :)

stephenwan-opal commented 1 year ago

Hi @baywet - I see the improvements in the non-beta sdk, but the beta-sdk (on version 0.49) still seems to get stuck. I let the compiler run for 10 minutes but it doesn't progress past the files in /github.com/microsoftgraph/msgraph-beta-sdk-go@v0.49.0/models according to go build -x -v ...

Has anybody else seen improvements with the beta sdk? My non-beta build time is extremely fast (<5s with no cache).

update: i let compile run for over an hour on my m1 macbook pro and it did not finish:

Screen Shot 2022-12-12 at 11 11 29 PM
$ go version
go version go1.19.4 darwin/arm64
rkodev commented 1 year ago

@stephenwan-opal I can confirm that beta release missed a few changes that are present in v1 but a new release for beta is scheduled in the next 2 days, please do check back on this and share your experience then

stephenwan-opal commented 1 year ago

@stephenwan-opal I can confirm that beta release missed a few changes that are present in v1 but a new release for beta is scheduled in the next 2 days, please do check back on this and share your experience then

Thanks! The new v0.50.0 for the beta is faster (down from > 1 hour), though I wouldn't say I'm happy with the build time yet (no cache):

go build -v -x aztest/main.go  400.92s user 154.75s system 291% cpu 3:10.94 total

edit: separately, I took a look at our overall go build compilation time after using the graph sdk and it seems like the build takes about 70% longer and uses 40% more CI resource (measured by Circle CI credits), which is pretty tough to swallow.

stephenwan-opal commented 1 year ago

edit: separately, I took a look at our overall go build compilation time after using the graph sdk and it seems like the build takes about 70% longer and uses 40% more CI resource (measured by Circle CI credits), which is pretty tough to swallow.

Just wanted to add more detail about my experience dealing with this.

debugging long build time

I tried to use kiota as mentioned earlier in this thread to generate a minimal sdk to use (see my steps here). Unfortunately, this did not have any meaningful impact to the build time. The core issue seems to be that the minimal sdk still has a huge models/ package - models are not pruned out at all and has >9k files.

On the compile step, building the models/ package takes a very long time (~7m for me on an m1 macbook pro, determined by reading the go build -x -v output). By itself, that's almost an okay issue if we can try to rely on the go build cache (i.e. the dep doesn't change often, hopefully you have go build cache working fine locally and in ci/cd).

On the linker step, the linker spends about 2-4x the amount of time. I suspect this is because it has to spend a while shaking out and deleting the unused models/ code. This time was a dealbreaker for me, since it is redone from scratch on every compile. A local development rebuild goes from 2-4s to 8-12s on my codebase.

tl; dr: I believe the big models/ package is a major offender and should probably be broken up.

current mitigation

As for my current mitigation, I ended up cutting build and link time by doing rm -rf models/ and then undeleting code until my build passed (required manually editing some models files too). This was pretty painful (and obviously a maintenance nightmare) so I don't know that I'd recommend it, but my build times are now ~unchanged from prior to using this sdk.

Alternatively, I wonder if manually editing the openapi description to remove unused fields would have been better, but it seems like the kiota download for the msgraph api is broken right now...

baywet commented 1 year ago

Thank you everyone for the great feedback you've provided. It's super helpful to us getting things right.

Reducing the size of this SDK further

We've reached an agreement across the board, /me will be mapped to /users/{id} to reduce the size further. This is being tracked here #347 This is most likely the last performance improvement we'll be doing before GA of these v1 and beta full clients based on the feedback we've received to date. Please understand that most of the team is out right now, this is most likely going to take a few weeks.

Kiota download failing

Thanks for reporting this, we have a fix for that here https://github.com/microsoft/kiota/pull/2097

models is enormous

This part is really for people interested in the technical details. TL;DR; metadata is not accurate enough to avoid pulling things unnecessarily and while we're working on fixing that, we're also looking into improving the "slicing" mechanism at generation for the self-served experience.

challenges at design time

Microsoft Graph leverages OData for its modeling and API definition. This standard introduces a high level of connectivity between "entities", which is a great thing when you're designing a "graph" API with a promise of being able to navigate between the data nodes. It however poses a few challenges from a client perspectives. A number of efforts are ongoing to help reduce the pain.

At the design level, namespacing was introduced on Microsoft Graph a couple of years ago, this is why some models now live under models/termStore, etc... If we can't move the existing because it'd break clients, we can at least design in a way that doesn't make the situation worse.

At the operations level, the metadata needs to be more accurate. A navigation property between two entities is materialized by two things:

Due to intricacies of how the service is built behind the scenes it is sometimes impossible to populate the navigation property value (e.g. even /me?$expand=drive won't get you anything in the drive property of the retrieved user). We have developed metadata elements to tell "you can navigate, but not retrieve this property, so no need to generate a code property on the model". The problem now is that we need to backtrack 7 years of edge cases to add that information, this is going to take years. Removing those "useless" properties from the models will not only reduce the amount of code being produced, but also save time during the linking process and save confusion for users (why is this property never populated?)

challenges at generation time

Kiota only generates models that are required by the selected endpoints, recursively. Because of the design challenge and the inaccuracies of the metadata I described above, it ends up generating a lot of models even with a few endpoints. Our PowerShell generation story has a more advanced slicing mechanism and we're looking into what could be applied from it to kiota to improve that aspect.

dadams39 commented 1 year ago

For Beta there does not seem to be any open issues addressing the build times. Are there any specific updates / forecast concerning Beta?

Follow-up: Are there open items that members can assist with within the Beta project?

baywet commented 1 year ago

beta being generated with the same generator as this one it benefits from the same improvements we've been working on here.

Follow-up: Are there open items that members can assist with within the Beta project?

Are you asking whether you can contribute to the beta library?

dadams39 commented 1 year ago

Are you asking whether you can contribute to the beta library?

stephenwan-opal commented on their steps for current mitigation. It was quite involved and not really sustainable. I was wondering if there were some manual changes for Betaor otherwise, that could assist in reducing the inaccuracies in these endpoints.

baywet commented 1 year ago

this remains the most up to date description of the situation. Beta is ahead of v1 in terms of content (what endpoints, models, etc...) because it previews the changes to come. This is also why it's bigger than v1. But both rely on the same generation steps and will deliver a similar experience. (as well as self-served clients generated with Kiota). As for the metadata being used for generation, steps are being taken internally to improve the accuracy, but it'll take time. I hope that clarifies the situation?

wizedkyle commented 1 year ago

I am currently hitting an issue with GitHub Actions when running go build/go test it hits github.com/microsoftgraph/msgraph-sdk-go/models and then the worker terminates I am assuming the worker has run out of resources. Currently i am using msgraph-sdk-go v0.50.0 and msgraph-beta-sdk-go v0.51.0.

Locally the performance isn't horrible however, it is running on an i9/16GB RAM image

Has anyone seen a similar issue with GitHub Actions with these recent versions? If i remove msgraph everything builds/tests fine.

mblaschke commented 1 year ago

@wizedkyle as a workaround enable swap:

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2

      - name: Set Swap Space
        uses: pierotofy/set-swap-space@master
        with:
          swap-size-gb: 12
wizedkyle commented 1 year ago

Thank you for the suggestion @mblaschke, the build now runs for about 1hr 30mins and then crashes from the same issue.

I am thinking I might need to move away from using the SDK.

dadams39 commented 1 year ago

Just @mblaschke, I was able to use the workaround. It took 2h 3m 11s; but it did work. Very helpful; thank you!

friedrichg commented 1 year ago

I left this issue a long time ago, but for some reason forgot to unsubscribe. I feel the pain most users have here so I wanted to share an alternative https://github.com/friedrichg/go-msgraph-example

baywet commented 1 year ago

Hi everyone, With v0.56.0 we've shipped two performance related improvements:

Note: we're still working on the release for Beta, but we anticipate these improvements will yield even better results since this version has many more models.

With those changes in place, we're seeing an additional 20% build time performance improvements.

This is the last set of changes we're making on that front since we've exhausted all avenues to improve build times which do not lead to a feature trade-off and since build performance is now acceptable in most development environments.

If build time performance or asset size are still challenging for your scenario, you might want to consider kiota the generator that powers this SDK. You'll be able to generate a client with the same development experience, but with only the endpoints your application needs. Since the early days, we're embedded the search experience for API descriptions and the filtering capabilities directly within kiota thanks to your (and others') feedback.

In terms of future plans, we're still working to fix our metadata to remove references between models when it actually doesn't apply. We're also working to embed Kiota (or the Microsoft Graph self-serve story) in GUI experiences (nothing to disclose at this time).

I'll leave this issue open for another couple of days, and then close it depending on the replies.

To finish: The whole team would like to thank all of you on this thread for your feedback and helping shape the Go SDK! You truly had an impact on design decisions, and the final experience.

ghost commented 1 year ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

breml commented 1 year ago

I am no longer using this package, so I am not in a good position to provide meaningful feedback on this, sorry.

fabiante commented 1 year ago

@baywet Thanks for you summary. I understand optimizing this issue is no simple feat. At the company I work at we will have to heavily rely on Graph API and therefore apprechiate your efforts.

If build time performance or asset size are still challenging for your scenario, you might want to consider kiota the generator that powers this SDK.

If in any case build times will be too high we will consider using Kiota too. Maybe, this fact is important enough to state in the projects readme or documentation...

baywet commented 1 year ago

Thanks for the suggestion. We have some work to simplify the self-serve story before it's ready to be broadly shared in the docs, but yes, it's part of the plan.

stephenwan-opal commented 1 year ago

hi @baywet, first, thanks for the work y'all have put into investigating this issue.

Unfortunately, the package is still not really in a usable place for us. From my previous comment, the initial build is down from 7m to 3m on my local machine (better! but still pretty bad for a single dependency). Rebuilds (measuring compiler linking time only) actually looks like it's gone up from 8-12s to 18-20s (baseline 4-6 seconds without this package). I'm using the v0.50.0 beta release.

baywet commented 1 year ago

Thanks for the numbers. Could you try with 56 or 57, we've implemented additional improvements as I was mentioning earlier

baywet commented 1 year ago

Sorry 55 in case of beta