Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SERF-498] Upgrade gorm to v2 #58

Merged
merged 12 commits into from Feb 17, 2023
Merged

[SERF-498] Upgrade gorm to v2 #58

merged 12 commits into from Feb 17, 2023

Conversation

laynax
Copy link
Contributor

@laynax laynax commented Oct 12, 2022

Description

Works done:

  • Replaced github.com/jinzhu/gorm with gorm.io/gorm (no go mod tidy yet)
  • Replaced different drivers with gorm related drivers under gorm.io/driver/*
  • Replaced custom gorm logger with gorm.io/gorm/logger along with tests.
  • Introduced sqlmock, this way we can test logger more properly.

Testing considerations

Running docker-compose run --rm sdk mage test:run must be smooth.

Running new sdk update on chassis must go OK too:

go get github.com/scribd/go-sdk@laynax/SERF-498/upgrade-gorm
go mod tidy
docker-compose run --rm server mage test:run

Checklist

  • Prefixed the PR title with the JIRA ticket code
  • Performed simple, atomic commits with [good commit messages][commit messages]
  • Verified that the commit history is linear and commits are squashed as necessary
  • Thoroughly tested the changes in development and/or staging
  • Updated the README.md as necessary

Related links

@laynax laynax force-pushed the laynax/SERF-498/upgrade-gorm branch from 4f40d42 to afb4ef2 Compare October 12, 2022 13:06
@laynax laynax changed the title [ WIP ] Partial work on upgrading gorm to v2 [SERF-498] Upgrade gorm to v2 Oct 12, 2022
@laynax laynax marked this pull request as ready for review October 12, 2022 13:17
@laynax laynax requested a review from a team as a code owner October 12, 2022 13:17
@laynax laynax force-pushed the laynax/SERF-498/upgrade-gorm branch 3 times, most recently from 1c933b4 to e0ab410 Compare October 12, 2022 14:16
Copy link
Contributor

@Neurostep Neurostep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 👍 gorm introduced lots of changes and improvements

I did a first quick pass and placed some comments 💬

pkg/database/gorm.go Outdated Show resolved Hide resolved
pkg/database/gorm.go Outdated Show resolved Hide resolved
pkg/database/gorm.go Outdated Show resolved Hide resolved
pkg/database/gorm.go Show resolved Hide resolved
pkg/instrumentation/database.go Outdated Show resolved Hide resolved
pkg/logger/gorm.go Outdated Show resolved Hide resolved
pkg/logger/logrus.go Outdated Show resolved Hide resolved
pkg/middleware/database_logging.go Show resolved Hide resolved
pkg/interceptors/database_logging.go Show resolved Hide resolved
pkg/interceptors/database_logging.go Show resolved Hide resolved
@laynax laynax force-pushed the laynax/SERF-498/upgrade-gorm branch from e0ab410 to 7f1ce3f Compare October 13, 2022 09:47
@laynax laynax requested a review from Neurostep October 13, 2022 09:50
pkg/database/gorm.go Outdated Show resolved Hide resolved
pkg/database/gorm.go Show resolved Hide resolved
pkg/database/gorm.go Outdated Show resolved Hide resolved
pkg/database/gorm.go Outdated Show resolved Hide resolved
pkg/instrumentation/database.go Outdated Show resolved Hide resolved
pkg/logger/gorm.go Outdated Show resolved Hide resolved
pkg/tracking/sentry_test.go Outdated Show resolved Hide resolved
pkg/logger/gorm_test.go Outdated Show resolved Hide resolved
pkg/logger/gorm_test.go Show resolved Hide resolved
pkg/logger/gorm.go Outdated Show resolved Hide resolved
@laynax laynax force-pushed the laynax/SERF-498/upgrade-gorm branch from 7f1ce3f to cfa3d41 Compare October 14, 2022 12:01
@laynax laynax requested a review from Neurostep October 14, 2022 12:02
@laynax laynax force-pushed the laynax/SERF-498/upgrade-gorm branch from cfa3d41 to 76160a6 Compare October 14, 2022 12:57
Neurostep
Neurostep previously approved these changes Oct 14, 2022
Copy link
Contributor

@Neurostep Neurostep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀 Great work 👍

I would wait for another reviewer to approve, maybe they could spot something else 👀

Also, very good idea to test this PR in real life application (document-search) in a development environment before merging. Check logs, traces and overall update ✅

Neurostep
Neurostep previously approved these changes Oct 14, 2022
Copy link
Contributor

@Neurostep Neurostep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

fotos
fotos previously approved these changes Oct 14, 2022
Copy link
Member

@fotos fotos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM 🚀

pkg/instrumentation/database.go Outdated Show resolved Hide resolved
pkg/logger/gorm.go Outdated Show resolved Hide resolved
pkg/logger/gorm.go Outdated Show resolved Hide resolved
Neurostep
Neurostep previously approved these changes Oct 14, 2022
Copy link
Contributor

@Neurostep Neurostep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@laynax laynax dismissed stale reviews from Neurostep and fotos via ff0da41 October 14, 2022 15:19
@laynax laynax force-pushed the laynax/SERF-498/upgrade-gorm branch from 56a8297 to ff0da41 Compare October 14, 2022 15:19
@laynax
Copy link
Contributor Author

laynax commented Feb 16, 2023

@fotos

Shouldn't we have the SQL as the message, like it was before (#58 (comment))? thinking If I remember correctly DataDog likes it better this way (when it displays the logs) but we have to (double) check that as well.
Overall I'm not sure I understand why we moved the SQL to a different key – was there a technical limitation or are the logs better displayed this way in DataDog?

Thanks for the review 🙏 (with a bit of a delay :) )
I don't remember the reason behind changing the key honestly, maybe I wanted to copy the way it's done in gorm default logger 🤷‍♂️
I reverted it back to the old sql key.

Also you mind sharing a DataDog link about SQL logging field, please?

pkg/logger/gorm.go Outdated Show resolved Hide resolved
@laynax
Copy link
Contributor Author

laynax commented Feb 16, 2023

To give an update on my comment regarding gorm's behavior on logging PII:
Having the Go code gormDB.Exec("SELECT id FROM users WHERE name = ?", "asd") will result in:

  • No log on any log level than trace.
  • Trace level, with error:
    {"level":"trace","message":"gorm DB Logger","sql":{"affected_rows":1,"duration":274989,"sql":"SELECT id FROM users WHERE name = ?"},"timestamp":"2023-02-16T10:51:05Z"}
  • Trace level, without error:
    {"error":"test error","level":"error","message":"gorm DB Logger","sql":{"affected_rows":0,"duration":350073,"sql":"SELECT id FROM users WHERE name = ?"},"timestamp":"2023-02-16T10:51:05Z"}

@laynax laynax force-pushed the laynax/SERF-498/upgrade-gorm branch 2 times, most recently from cf33e2a to a9c3877 Compare February 17, 2023 13:03
pkg/logger/logger.go Outdated Show resolved Hide resolved
pkg/logger/logrus.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Neurostep Neurostep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@laynax
Copy link
Contributor Author

laynax commented Feb 17, 2023

For posterity, here's a log example in document-search running on trace log level.

@laynax laynax merged commit cccd416 into main Feb 17, 2023
@laynax laynax deleted the laynax/SERF-498/upgrade-gorm branch February 17, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants