[LogStreaming] App logs command #4041

mssalemi · 2024-06-12T19:17:35Z

Part of the LogStreaming Prototype project.

#gsd:39776

Current:

 NODE_TLS_REJECT_UNAUTHORIZED=0  SHOPIFY_CLI_ENABLE_APP_LOG_POLLING=1 pnpm shopify app logs --path ~/code/log-streaming/mehdi-local-cli --json --status=failure --source=product-discount

WHY are these changes introduced?

We are adding the app logs command. This command is similar to dev, but will stream logging events with full detail. It will not write logs to file. See the Nick's DX doc for more details.

Log Output

Error State

WHAT is this pull request doing?

The <Logs /> component will handle both the polling, and displaying the output.

The log and subscribe process are now only responsible for making the requests. This processes are used in the Logs component and are results are handling by either adding the log to the output, or displaying the errors.

TLDR on the error handling:

If errors are 401 -> we do not update the polling interval, we will re-subscribe. Manage this in the react component by setting the JWT state to null, so the next time it polls, it will re-subscribe first.

If errors are 429 -> update the polling interval to 60 seconds. We also show the error state and a retry in 60 seconds message.

If errors are we update the retry interval to 5 second, and display the error message.

If errors are not handled, they will get thrown. This will cause the terminal to abort.

If there are app logs, they are displayed to the CLI through the Static component. This component is also used by dev's ConcurrentOutput.

How to test your changes?

Checkout this branch

Run:

NODE_TLS_REJECT_UNAUTHORIZED=0  SHOPIFY_CLI_ENABLE_APP_LOG_POLLING=1 pnpm shopify app logs --path ~/code/log-streaming/mehdi-local-cli

Optionally:
(https://github.com/Shopify/partners/pull/55154 needed)

--status=failure --source=extension-handle

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes
I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

github-actions · 2024-06-12T19:17:53Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/advanced-edits
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/app-management

github-actions · 2024-06-12T19:21:52Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.45% (-0.17% 🔻)	7485/10331
🟡	Branches	69.11% (-0.11% 🔻)	3676/5319
🟡	Functions	71.59% (-0.01% 🔻)	1983/2770
🟡	Lines	72.77% (-0.2% 🔻)	7066/9710

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / logs.ts	0%	0%	0%	0%
🔴	`...` / logs.ts	0%	0%	0%	0%
🟢	`...` / poll-app-logs.ts	100%	82.35%	100%	100%
🔴	`...` / ui.tsx	0%	100%	0%	0%
🟢	`...` / Logs.tsx	100%	87.5%	100%	100%
🟢	`...` / usePollAppLogs.ts	94.29%	92.86%	80%	93.55%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / flow_template.ts	33.33% (-5.13% 🔻)	0%	33.33% (+13.33% 🔼)	33.33% (-5.13% 🔻)
🟢	`...` / utils.ts	86.36% (-8.37% 🔻)	75% (-5% 🔻)	100%	86.36% (-8.37% 🔻)
🟢	`...` / poll-app-logs.ts	88.37% (-3.01% 🔻)	81.25%	44.44% (-10.1% 🔻)	88.37% (-3.01% 🔻)
🟢	`...` / breakdown-extensions.ts	96.55% (-0.14% 🔻)	83.33%	100%	100%
🟢	`...` / upload.ts	84.09% (-0.87% 🔻)	78.57% (-1.98% 🔻)	85.37%	84.87% (-0.96% 🔻)
🟢	`...` / replay.ts	97.06% (+0.57% 🔼)	80% (-6.96% 🔻)	100%	97.06% (+0.57% 🔼)

Test suite run success

1723 tests passing in 800 suites.

Report generated by 🧪jest coverage report action from 73001ce

DuncanUszkay1

Mainly just some nits on my end, though I'd want CLI folks to take a look before I put the ✅ since there are some elements in here that look like they should probably be generic 🤔

packages/app/src/cli/services/app-logs/ui/components/Logs.tsx

packages/app/src/cli/services/context.ts

packages/app/src/cli/commands/app/logs.ts

packages/app/src/cli/services/app-logs/processes/polling-app-logs.ts

packages/app/src/cli/services/app-logs/ui/components/Logs.tsx

packages/app/src/cli/services/app-logs/processes/polling-app-logs.ts

packages/app/src/cli/services/context.ts

packages/app/src/cli/services/app-logs/processes/polling-app-logs.ts

packages/cli-kit/src/public/node/ink.ts

…tor changes

DuncanUszkay1 · 2024-06-20T19:14:16Z

packages/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.ts

+            logTimestamp: log.log_timestamp,
+          }
+
+          setAppLogOutputs((prev) => [...prev, {appLog, prefix}])


Curious if we need to keep track of every event we've ever seen 🤔 Not urgent but something to explore as a fast-follow

Since we're using React, which is state-based rendering, I suspect that we need to keep an array of all lines that we want to render on the screen.

That's true in the browser, but is it true with ink? Does dev keep track of every line it's ever printed? If not, wouldn't we be able to do something similar?

Might just be a gap in my understanding of ink

FWIW The output component dev (and this) uses does keep the entire output in state -- it doesn't do virtualisation or anything like that

packages/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.test.tsx

github-actions · 2024-06-20T19:22:43Z

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

packages/app/src/cli/commands/app/logs.ts

packages/app/src/cli/services/app-logs/logs-command/poll-app-logs.test.ts

packages/app/src/cli/services/app-logs/logs-command/poll-app-logs.ts

packages/app/src/cli/services/app-logs/types.ts

andrewhassan · 2024-06-20T20:11:02Z

packages/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.ts

+            logTimestamp: log.log_timestamp,
+          }
+
+          setAppLogOutputs((prev) => [...prev, {appLog, prefix}])


Since we're using React, which is state-based rendering, I suspect that we need to keep an array of all lines that we want to render on the screen.

andrewhassan · 2024-06-20T20:15:22Z

packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.tsx

+              <Text color="green">{prefix.logTimestamp}</Text>
+              <Text color="blueBright">{`${prefix.source}`}</Text>
+              <Text color={prefix.status === 'Success' ? 'green' : 'red'}>{prefix.status}</Text>
+              <Text>in {prefix.fuelConsumed} M instructions</Text>


❔ Should we keep the prefix generic for other app log types? In this case, should the prefix actually have something like description (set to the instruction count for Functions) or something that we just blindly output here?

packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.test.tsx

packages/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.ts

shauns

This is looking better now, but still a few things to pick up. I think Andrew's round of review in particular does need to be actioned before merging.

packages/app/src/cli/commands/app/logs.ts

shauns · 2024-06-21T10:08:29Z

packages/app/src/cli/commands/app/logs.ts

+      env: 'SHOPIFY_FLAG_SOURCE',
+    }),
+    status: Flags.string({
+      description: 'Filters output to the specified status (success or failure).',


If these are fixed strings, lets use options as per the above.

Ah yea, the status will be fixed to success or failure, whereas the source is the extension handle. Maybe we could do @nickwesselman?

static descriptionWithMarkdown = ` Opens a real-time stream of detailed log events from the selected app and store. Use the \`--source\` argument to limit output to a particular function, such as a specific Shopify Function handle. Use the \`--status\` argument to specify the type of status to retrieve, either \`success\` or \`failure\`. \`\`\` shopify app logs --status=success --source=extension-handle \`\`\` `

We will update the flags to use the options for status.

packages/app/src/cli/commands/app/logs.ts

packages/app/src/cli/services/app-logs/logs-command/poll-app-logs.test.ts

shauns · 2024-06-21T10:14:25Z

packages/app/src/cli/services/app-logs/logs-command/poll-app-logs.ts

+  cursor?: string
+  errors?: {
+    status: number
+    message: string
+  }[]
+  appLogs?: AppLogData[]


shauns · 2024-06-21T10:21:08Z

packages/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.ts

+
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises
+    poll()
+  }, [])


Do we need to handle unmount here -- cancelling the timeout?

…led polling

mssalemi force-pushed the ms.app-logs-command-version2-no-refactor branch 2 times, most recently from d976073 to 1586bed Compare June 12, 2024 21:29

DuncanUszkay1 reviewed Jun 13, 2024

View reviewed changes

shauns requested changes Jun 13, 2024

View reviewed changes

mssalemi force-pushed the ms.app-logs-command-version2-no-refactor branch 5 times, most recently from 796cc88 to 68adb58 Compare June 14, 2024 12:24

mssalemi commented Jun 14, 2024

View reviewed changes

packages/cli-kit/src/public/node/ink.ts Outdated Show resolved Hide resolved

mssalemi force-pushed the ms.app-logs-command-version2-no-refactor branch from ddff241 to a982bee Compare June 18, 2024 01:32

mssalemi changed the base branch from main to lopert.polling-refactor June 18, 2024 01:32

mssalemi force-pushed the ms.app-logs-command-version2-no-refactor branch 3 times, most recently from a697f66 to 2b6fef6 Compare June 18, 2024 14:04

lopert force-pushed the lopert.polling-refactor branch from da11633 to 764ab1d Compare June 18, 2024 14:32

Base automatically changed from lopert.polling-refactor to main June 18, 2024 14:45

mssalemi added 11 commits June 19, 2024 09:35

add boilerplate for logs command

8c046e3

Add <Log /> component, and poll from the react component

43c0b48

handle errors from polling in <Log />

3478730

Add re-subscribe, and polling backoff

e19f8d0

add filters to URL query

077b218

codereview: simplify using Dev context / flags

b208b18

generate docs

a18f4a4

add tests: subscribe and poll process, align code with rebase / refac…

6d74367

…tor changes

update response handling

3bbe6ce

Refactor without tests

6e1bb62

fix poll app logs for logs test

6fc9b8f

lopert and others added 2 commits June 20, 2024 13:13

refresh manifest

c242360

fix Logs test

a4d3220

mssalemi requested review from lopert, DuncanUszkay1 and shauns June 20, 2024 18:05

mssalemi added 2 commits June 20, 2024 14:15

hide logs command behind envVar

64aa698

remove generated docs

d2614e5

mssalemi force-pushed the ms.app-logs-command-version2-no-refactor branch from f4a6cf0 to d2614e5 Compare June 20, 2024 18:25

reorganize file structure

0eff461

mssalemi force-pushed the ms.app-logs-command-version2-no-refactor branch from f056e4d to 0eff461 Compare June 20, 2024 19:00

DuncanUszkay1 reviewed Jun 20, 2024

View reviewed changes

mssalemi commented Jun 20, 2024

View reviewed changes

packages/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.test.tsx Show resolved Hide resolved

DuncanUszkay1 approved these changes Jun 20, 2024

View reviewed changes

mssalemi marked this pull request as ready for review June 20, 2024 19:22

mssalemi enabled auto-merge June 20, 2024 19:23

lopert approved these changes Jun 20, 2024

View reviewed changes

add filtering

d5f09aa

andrewhassan reviewed Jun 20, 2024

View reviewed changes

mssalemi force-pushed the ms.app-logs-command-version2-no-refactor branch from 0a7b159 to d5f09aa Compare June 20, 2024 21:09

shauns reviewed Jun 21, 2024

View reviewed changes

shauns approved these changes Jun 21, 2024

View reviewed changes

mssalemi added this pull request to the merge queue Jun 21, 2024

shauns removed this pull request from the merge queue due to a manual request Jun 21, 2024

mssalemi added 4 commits June 21, 2024 08:26

code review updates andrew

d170a4e

code review shaun: fix flags, update poll app logs tests

1917908

unify env check across commands

e1d632f

refresah manifest

1a54e12

mssalemi changed the title ~~[LogStreaming] App logs command (without refactor)~~ [LogStreaming] App logs command Jun 21, 2024

update prefix type to show description + remove double check for enab…

73001ce

…led polling

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LogStreaming] App logs command #4041

[LogStreaming] App logs command #4041

mssalemi commented Jun 12, 2024 •

edited

Loading

github-actions bot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024 •

edited

Loading

DuncanUszkay1 left a comment

DuncanUszkay1 Jun 20, 2024

andrewhassan Jun 20, 2024

DuncanUszkay1 Jun 20, 2024 •

edited

Loading

DuncanUszkay1 Jun 20, 2024

shauns Jun 21, 2024

github-actions bot commented Jun 20, 2024

andrewhassan Jun 20, 2024

andrewhassan Jun 20, 2024

shauns left a comment

shauns Jun 21, 2024

mssalemi Jun 21, 2024

shauns Jun 21, 2024

shauns Jun 21, 2024

[LogStreaming] App logs command #4041

Are you sure you want to change the base?

[LogStreaming] App logs command #4041

Conversation

mssalemi commented Jun 12, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

Checklist

github-actions bot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024 • edited Loading

Coverage report

Test suite run success

DuncanUszkay1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DuncanUszkay1 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shauns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssalemi commented Jun 12, 2024 •

edited

Loading

github-actions bot commented Jun 12, 2024 •

edited

Loading

DuncanUszkay1 Jun 20, 2024 •

edited

Loading