CLOUDP-211581: Improve memory consumption in Atlas SDK #331

fmenezes · 2024-05-30T17:09:32Z

Description

Improve memory consumption in Atlas SDK (by not reading response.Body when we don't need)

Link to any related issue(s): CLOUDP-211581

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Required Checklist:

I have signed the MongoDB CLA
I have added tests that prove my fix is effective or that my feature works
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code

Further comments

fmenezes · 2024-05-31T11:17:04Z

running tests at mongodb/mongodb-atlas-cli#2979

wtrocki · 2024-06-03T10:15:37Z

.github/workflows/autoupdate-preview.yaml

@@ -13,6 +13,7 @@ jobs:
      - uses: actions/setup-go@v5
        with:
          go-version-file: 'go.mod'
+      - run: go install golang.org/x/tools/cmd/[email protected]


golangcilint wraps goimports for checks. Since we need it her for code fixing I think it is nicer to consider adding it to

make clean_and_generate

make tools

Basically [super nit] I think we still need it locally (as part of the git hooks - tools) and for generation.
Once we add it there would not be need for any autoupdate workflow changes.

Those are quite hard to test locally and might fail after merge.

it is already in make tools

wtrocki · 2024-06-03T10:16:19Z

githooks/pre-commit

@@ -4,7 +4,6 @@ STAGED_GO_FILES=$(git diff --cached --name-only | grep ".go$")

 for FILE in ${STAGED_GO_FILES}
 do
-    gofmt -w -s "${FILE}"


There are still some differences between gofmt and goimports ( goimports does less).
My guess is that we want to align SDK hooks in other golang repose like CLI?

goimports is needed because some imports have to be removed after generation to avoid compilation errors (e.g. io)

wtrocki · 2024-06-03T10:35:23Z

Verified locally:

Creation of AWS cluster

▶ go run ./aws_cluster/aws.go
SDK: Created new MongoDB cluster: 665d9c2735f85255276102f9 
Please wait up to 10 minutes for cluster to provision.

Getting file from backend

wtrocki · 2024-06-03T10:54:37Z

admin/api_atlas_search.go

-		}
-		newErr.error = formatErrorMessage(localVarHTTPResponse.Status, localVarHTTPMethod, localVarPath, v)
-		newErr.model = v
+		newErr := a.client.makeApiError(localVarHTTPResponse, localVarHTTPMethod, localVarPath)


wtrocki · 2024-06-03T10:56:36Z

admin/api_atlas_search.go

 	if err != nil {
+		defer localVarHTTPResponse.Body.Close()


[Question] Why closing only on error? Should each method be responsible for closing body independently from if decode succeeded or not?

close only happens in this case because it already happens in decode and makeApiError

wtrocki · 2024-06-03T11:02:56Z

When testing error case I noticed that resp.Body is not returned as pointer.
I modified aws.go to cause error by providing wrong region:

go run ./aws_cluster/aws.go  
{0x1400024a180}
https://cloud-dev.mongodb.com/api/atlas/v2/groups/65bd48ebe4cb1d1533f14fd6/clusters POST: HTTP 400 Bad Request (Error code: "INVALID_ATTRIBUTE") Detail: Invalid attribute regionName specified. Reason: Bad Request. Params: [regionName]
2024/06/03 12:57:28 Error when performing SDK request: Invalid attribute regionName specified.
exit status 1

On master that renders:

{{"detail":"Invalid attribute regionName specified.","error":400,"errorCode":"INVALID_ATTRIBUTE","parameters":["regionName"],"reason":"Bad Request"}}
https://cloud-dev.mongodb.com/api/atlas/v2/groups/65bd48ebe4cb1d1533f14fd6/clusters POST: HTTP 400 Bad Request (Error code: "INVALID_ATTRIBUTE") Detail: Invalid attribute regionName specified. Reason: Bad Request. Params: [regionName]
2024/06/03 13:01:38 Error when performing SDK request: Invalid attribute regionName specified.
exit status 1

Is there way to keep previous behaviour?

wtrocki · 2024-06-03T11:06:10Z

@fmenezes Awesome work on making this major improvement!

I have done some testing and left couple questions, but everything looks LGTM.
We need to clarify possibility of breaking change (as this might require some additional steps for that PR.

fmenezes · 2024-06-04T10:45:49Z

When testing error case I noticed that resp.Body is not returned as pointer. I modified aws.go to cause error by providing wrong region:

go run ./aws_cluster/aws.go  
{0x1400024a180}
https://cloud-dev.mongodb.com/api/atlas/v2/groups/65bd48ebe4cb1d1533f14fd6/clusters POST: HTTP 400 Bad Request (Error code: "INVALID_ATTRIBUTE") Detail: Invalid attribute regionName specified. Reason: Bad Request. Params: [regionName]
2024/06/03 12:57:28 Error when performing SDK request: Invalid attribute regionName specified.
exit status 1

On master that renders:

{{"detail":"Invalid attribute regionName specified.","error":400,"errorCode":"INVALID_ATTRIBUTE","parameters":["regionName"],"reason":"Bad Request"}}
https://cloud-dev.mongodb.com/api/atlas/v2/groups/65bd48ebe4cb1d1533f14fd6/clusters POST: HTTP 400 Bad Request (Error code: "INVALID_ATTRIBUTE") Detail: Invalid attribute regionName specified. Reason: Bad Request. Params: [regionName]
2024/06/03 13:01:38 Error when performing SDK request: Invalid attribute regionName specified.
exit status 1

Is there way to keep previous behaviour?

TL;DR we have to change our examples

Basically in our handleErr function we fmt.Println(resp.Body) instead we should have been printing err.body from generic error.

this is misleading because we can't tell what is the state of resp.Body after the body was already read.

wtrocki · 2024-06-04T10:51:06Z

Basically in our handleErr function we fmt.Println(resp.Body) instead we should have been printing err.body from generic error. this is misleading because we can't tell what is the state of resp.Body after the body was already read.

So basically we need documentation to not rely on the resp.Body for printing body from request and follow your advice.
If customers use that they might need to adjust their changes.
If that is correct we might want

Short note in error handling documentation (./docs folder)
Breaking change note - no need to make it here (we can reuse existing breaking change update PR as follow up
Update example

No strong opinion on those.

wtrocki · 2024-06-04T10:58:22Z

Will LGTM once we address the conflict (regenerate code). I'm ok to take follow up documentation actions if you want.

CLOUDP-211581: Improve memory consumption in Atlas SDK

4e88225

fmenezes requested a review from a team as a code owner May 30, 2024 17:09

fmenezes added 6 commits May 30, 2024 18:11

goimports

f40be83

pin goimports version

012e316

fix

9c808f9

fix

82f20e7

fix

5088a1d

fix

fc96c2d

fmenezes force-pushed the CLOUDP-211581 branch from f0d1c0b to fc96c2d Compare May 30, 2024 17:35

fix

bafc3f5

fmenezes added 3 commits May 31, 2024 14:25

fix lint

16b912f

fix lint on supporting files

bccb216

fix

da4d6c8

wtrocki reviewed Jun 3, 2024

View reviewed changes

fix: go mod updates for testing

16c5b68

wtrocki reviewed Jun 3, 2024

View reviewed changes

Merge branch 'main' into CLOUDP-211581

3c1af78

wtrocki approved these changes Jun 4, 2024

View reviewed changes

fmenezes merged commit 53bb4f8 into main Jun 4, 2024
4 checks passed

fmenezes deleted the CLOUDP-211581 branch June 4, 2024 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLOUDP-211581: Improve memory consumption in Atlas SDK #331

CLOUDP-211581: Improve memory consumption in Atlas SDK #331

fmenezes commented May 30, 2024

fmenezes commented May 31, 2024

wtrocki Jun 3, 2024 •

edited

Loading

wtrocki Jun 3, 2024

fmenezes Jun 4, 2024

wtrocki Jun 3, 2024

wtrocki Jun 3, 2024

fmenezes Jun 4, 2024

wtrocki commented Jun 3, 2024

wtrocki Jun 3, 2024

wtrocki Jun 3, 2024 •

edited

Loading

fmenezes Jun 4, 2024

wtrocki commented Jun 3, 2024

wtrocki commented Jun 3, 2024

fmenezes commented Jun 4, 2024

wtrocki commented Jun 4, 2024 •

edited

Loading

wtrocki commented Jun 4, 2024 •

edited

Loading

CLOUDP-211581: Improve memory consumption in Atlas SDK #331

CLOUDP-211581: Improve memory consumption in Atlas SDK #331

Conversation

fmenezes commented May 30, 2024

Description

Type of change:

Required Checklist:

Further comments

fmenezes commented May 31, 2024

wtrocki Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

wtrocki Jun 3, 2024

Choose a reason for hiding this comment

fmenezes Jun 4, 2024

Choose a reason for hiding this comment

wtrocki Jun 3, 2024

Choose a reason for hiding this comment

wtrocki Jun 3, 2024

Choose a reason for hiding this comment

fmenezes Jun 4, 2024

Choose a reason for hiding this comment

wtrocki commented Jun 3, 2024

wtrocki Jun 3, 2024

Choose a reason for hiding this comment

wtrocki Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

fmenezes Jun 4, 2024

Choose a reason for hiding this comment

wtrocki commented Jun 3, 2024

wtrocki commented Jun 3, 2024

fmenezes commented Jun 4, 2024

wtrocki commented Jun 4, 2024 • edited Loading

wtrocki commented Jun 4, 2024 • edited Loading

wtrocki Jun 3, 2024 •

edited

Loading

wtrocki Jun 3, 2024 •

edited

Loading

wtrocki commented Jun 4, 2024 •

edited

Loading

wtrocki commented Jun 4, 2024 •

edited

Loading