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

feat(migrate): write a log file of all submitted transactions #5757

Closed
wants to merge 1 commit into from

Conversation

bjoerge
Copy link
Member

@bjoerge bjoerge commented Feb 15, 2024

Description

This writes a log of all transactions submitted during a migration to a file within the migration directory. The logfile consists of newline delimited responses from the /mutate API endpoint. I'm going for a tradeoff between minimalistic and valuable here, and did not include the changed documents this time, because of the potential for filling up lots of disk space if running migrations on high volumes. We can always consider making it configurable behind a flag later.

Example output:

? This migration will run on the test dataset in ppsg7ml5 project. Are you sure? Yes
✔ Migration "rename-location-to-address" completed.

  Project id:  ppsg7ml5
  Dataset:     test

  678 documents processed.
  678 mutations generated.
  1 transactions committed.

A log of all submitted transactions has been written to:
./migrations/rename-location-to-address/logs/2024-02-15T16_44_39_628Z-rename-location-to-address.ndjson

☝🏼 The last two lines here are new

What to review

  • Does it make sense to make it a default to write a file, or should it be opt-in?
  • Does the flag name make sense?

Testing

  • Run a migration and have a look at the logfile.

Notes for release

  • sanity migrate now logs all submitted transactions to a file during a migration run.

@bjoerge bjoerge requested a review from a team as a code owner February 15, 2024 16:50
@bjoerge bjoerge requested review from cngonzalez and removed request for a team February 15, 2024 16:50
Copy link

vercel bot commented Feb 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Feb 16, 2024 8:41am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 16, 2024 8:41am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Feb 16, 2024 8:41am

const stats: MigrationProgress = {
documents: 0,
mutations: 0,
pending: 0,
queuedBatches: 0,
completedTransactions: [],
completedTransactionsLength: 0,
Copy link
Member Author

Choose a reason for hiding this comment

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

this wasn't strictly needed for this PR, but we didn't use the array for anything else than reading it's length, so thought I'd might as well free up some memory.

Copy link
Contributor

github-actions bot commented Feb 15, 2024

Package Documentation Change
@sanity/portable-text-editor +5%
sanity -3%
@sanity/diff -55%
Full Report
@sanity/portable-text-editor
This branch Next branch
21 documented 20 documented
44 not documented 45 not documented
@sanity/migrate
This branch Next branch
14 documented 14 documented
78 not documented 62 not documented
@sanity/block-tools
This branch Next branch
4 documented 4 documented
9 not documented 9 not documented
@sanity/types
This branch Next branch
55 documented 55 documented
233 not documented 239 not documented
sanity/desk
This branch Next branch
84 documented 84 documented
64 not documented 64 not documented
@sanity/mutator
This branch Next branch
7 documented 7 documented
3 not documented 4 not documented
@sanity/cli
This branch Next branch
1 documented 1 documented
31 not documented 31 not documented
sanity/structure
This branch Next branch
2 documented 2 documented
8 not documented 8 not documented
@sanity/util/concurrency-limiter
This branch Next branch
1 documented 1 documented
0 not documented 0 not documented
@sanity/util/legacyDateFormat
This branch Next branch
0 documented 0 documented
4 not documented 5 not documented
@sanity/schema/_internal
This branch Next branch
0 documented 0 documented
12 not documented 12 not documented
@sanity/util/paths
This branch Next branch
1 documented 1 documented
15 not documented 15 not documented
sanity/router
This branch Next branch
17 documented 17 documented
26 not documented 26 not documented
@sanity/schema
This branch Next branch
0 documented 0 documented
2 not documented 2 not documented
sanity/cli
This branch Next branch
2 documented 2 documented
0 not documented 0 not documented
@sanity/vision
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
@sanity/util/fs
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
sanity/_internal
This branch Next branch
0 documented 0 documented
1 not documented 1 not documented
@sanity/util/client
This branch Next branch
1 documented 1 documented
0 not documented 0 not documented
@sanity/util/createSafeJsonParser
This branch Next branch
1 documented 1 documented
0 not documented 0 not documented
sanity/_internalBrowser
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
@sanity/util/content
This branch Next branch
1 documented 1 documented
5 not documented 5 not documented
sanity
This branch Next branch
178 documented 185 documented
840 not documented 842 not documented
@sanity/diff
This branch Next branch
13 documented 29 documented
16 not documented 0 not documented

Copy link
Contributor

github-actions bot commented Feb 15, 2024

Component Testing Report Updated Feb 16, 2024 8:44 AM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 31s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 12s 3 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 13s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 32s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 18s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 59s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 19s 9 0 0

@bjoerge bjoerge force-pushed the sdx-995/migration-report branch from 489c73f to 930f2bb Compare February 15, 2024 17:04
Copy link
Member Author

bjoerge commented Feb 15, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bjoerge and the rest of your teammates on Graphite Graphite

Copy link
Member

@cngonzalez cngonzalez left a comment

Choose a reason for hiding this comment

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

I did successfully generate a logfile and personally feel that doing it by default makes sense, especially as this feature is so new.

I'd like us to surface the no-transaction-log flag pretty quickly to customers, I can see this being a possible footgun since it's not particularly visible and our docs on this are pretty verbose.

I also think the more verbose log return will be a pretty quick feature request but likely more as a series of diffs.

@bjoerge
Copy link
Member Author

bjoerge commented Feb 16, 2024

Thanks for the review!

I can see this being a possible footgun since it's not particularly visible and our docs on this are pretty verbose.

Not sure I understand what you mean by this. What's the footgun here?

I also think the more verbose log return will be a pretty quick feature request but likely more as a series of diffs.

Agree! I like the idea of logging as a diff against the current/before-document. It's a slightly more involved task, so I'll file it as a feature request for now, so we can track signal.

@cngonzalez
Copy link
Member

I can see this being a possible footgun since it's not particularly visible and our docs on this are pretty verbose.

Not sure I understand what you mean by this. What's the footgun here?

Apologies! "Footgun" was maybe too strong a word here. "Possible inconvenience" or "source of confusion" would have been more accurate. Here's my concern more explicitly:

I expect that a number of developers will run these on sometimes high amounts of documents, generating logfiles with tens of thousands of lines each. As far as I can tell, we don't really offer any advice on how these logfiles fit within a typical development workflow. Is it annoying when I accidentally commit these to Git? Or am I supposed to commit these? In what situations should I or should I not keep these logfiles around?

So for me, a small source of trouble is really just generating large-ish files and possibly having to keep them around by default. I don't think it's a big issue at all, but just one of those places where I think users would appreciate an opinionated workflow and rationale for what they should do with the things we're generating for them.

@bjoerge bjoerge marked this pull request as draft March 5, 2024 10:08
@bjoerge
Copy link
Member Author

bjoerge commented Mar 5, 2024

These are good points and I agree with you @cngonzalez. Open for suggestions on how to proceed, but I'm thinking an acceptable path forward would be to:

  • Make the logfile opt-in instead of opt-out
  • Make it possible to specify the directory where the logfile should be written

e.g.:

sanity migrate --transaction-log --transaction-log-dir=/tmp …

That should remove some of the potential confusion I would hope? What to do with logfiles in terms of git/source control is something we should still document (I'd say they should be ignored).

@bjoerge
Copy link
Member Author

bjoerge commented Aug 14, 2024

Closing this for now - might pick it up at a later point

@bjoerge bjoerge closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants