[ShopifyVM] 'app logs' Show input query variables in output for FunctionRunLog #4566

mssalemi · 2024-10-02T19:25:52Z

Part of: https://github.com/Shopify/script-service/issues/6233

We're enhancing the debugging experience for developers by providing visibility into the input query variables used during a function run.

WHY are these changes introduced?

This PR will add an input query variables section that will conditionally show based on if input query variables are used. Input query variables will be used when the values are specified in the toml.

WHAT is this pull request doing?

Add input query variables section to the app logs command:

I also added some visual improvements to make the output and input sections more clear for partners to view. And updated the ouput and input sections to match.

Changes: with improved styling

With coloring (also will need to update other sections - alternate idea)

and when no metafield is found:

How to test your changes?

From local cli, to your spin instance:

SHOPIFY_CLI_ENABLE_APP_LOG_POLLING=1  NODE_TLS_REJECT_UNAUTHORIZED=0 SPIN=1 SPIN_INSTANCE=68fh pnpm run shopify app logs --path=./efficient-capital-app

** replace spin id and app path

Post-release steps

part of a CLI release
potentially add change log indicating that the input query variables are now present in logs and has visual improvements
use of colouring in text, I kept everything white in current commit, but use red text when metafield is not found.
NOTE: might be worth splitting up logic for displaying FunctionRunLog and NetworkAccessResponseFromCacheLog, NetworkAccessRequestExecutedLog into separate files.

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

github-actions · 2024-10-02T19:26:10Z

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-inner-loop

github-actions · 2024-10-03T14:26:22Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.64% (+0.03% 🔼)	8379/11535
🟡	Branches	69.19% (+0.08% 🔼)	4124/5960
🟡	Functions	71.92% (-0.02% 🔻)	2190/3045
🟡	Lines	72.95% (+0.03% 🔼)	7922/10860

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / utils.ts	87.25% (-0.11% 🔻)	74.51% (+2.17% 🔼)	82.35% (-5.15% 🔻)	86.87% (-0.09% 🔻)

Test suite run success

1911 tests passing in 867 suites.

Report generated by 🧪jest coverage report action from b318ad7

github-actions · 2024-10-03T14:43:27Z

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/services/app-logs/logs-command/ui/components/Logs.test.tsx

andrewhassan · 2024-10-08T13:47:06Z

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

+ <Box marginLeft={1} marginTop={1}>
+ <Text>
+ {prettyPrintJsonIfPossible(appLog.inputQueryVariablesMetafieldValue) || (
+ <Text color="red">No metafield data found!</Text>


Maybe another way of stating this is: Metafield is not set

@nickwesselman WDYT of this?

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

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

DuncanUszkay1

LGTM- just the one issue with a lot of use of the word webhook in the tests that I think isn't appropriate- network access relates to network access in functions (though given how often we talk about webhooks in this project and the naming here I can see where the confusion would come from)

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

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

andrewhassan

Didn't 🎩 but code LGTM!

mssalemi · 2024-10-17T17:29:51Z

Quick video if anyone wants to take a look at changes. Will rebase PR before merging.

nickwesselman · 2024-10-17T17:37:30Z

Thanks for the video @mssalemi! Things look good in app logs, how do they look in the JSON log file that you might open when using app dev?

mssalemi · 2024-10-17T18:06:08Z

Thanks for the video @mssalemi! Things look good in app logs, how do they look in the JSON log file that you might open when using app dev?

The variables get added to the payload in the logs:

  "payload": {
.....
    "inputQueryVariablesMetafieldValue": "{\"value\":\"42\"}",
    "inputQueryVariablesMetafieldNamespace": "meow",
    "inputQueryVariablesMetafieldKey": "key"
  }

perhaps we want to decode the the value 👀

nickwesselman · 2024-10-17T18:10:50Z

The variables get added to the payload in the logs:

  "payload": {
.....
    "inputQueryVariablesMetafieldValue": "{\"value\":\"42\"}",
    "inputQueryVariablesMetafieldNamespace": "meow",
    "inputQueryVariablesMetafieldKey": "key"
  }

Can we attempt to parse the value and output it as an object if successful?

jamieguerrero · 2024-10-17T18:24:08Z

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

- } else if (typeof json === 'object' && json !== null) {
- return JSON.stringify(json, null, 2)
- } else {
+ if (typeof json === 'string') {


Could you explain the reasoning for this change here?

Will look into this, its related to Nicks comment above. Right now with PR we dont decode that part of the payload, so to display the JSON string better I added that. But thinking, it probably makes more sense to decode the inner payload here. Will look into it.

Would be great if you could add a return type so it's easier to grok what this function is trying to return!

My main question is why you removed the other cases from within the try/catch block instead of just changing the throw to return json.

Additionally, why are we changing this to return json instead of throwing an error?

Ah sounds good, reverting this change. I changed up how I call this method and ensure the metafield is an object, that we can we leave this unchanged.

why are we changing this to return json instead of throwing an error?

Reverted this, but originally was because this values the we needed to use this method on is from a metafield, and it could technically just be a string, and we dont want to error in those cases and just show whatever the value was. That way partners can use it to debug any issue related to the input query variables.

…ject

mssalemi · 2024-10-17T19:49:25Z

Can we attempt to parse the value and output it as an object if successful?

Updated this so we the value is object instead of string, partners should be able to more easily read them.

mssalemi changed the title ~~[WIP] Show input query variables in app logs command for function runs~~ [ShopifyVM] 'app logs` Show input query variables in output for function runs Oct 3, 2024

mssalemi force-pushed the ms.app-logs-add-iqv-output branch from 7c60a20 to 685540c Compare October 3, 2024 14:35

mssalemi marked this pull request as ready for review October 3, 2024 14:43

mssalemi changed the title ~~[ShopifyVM] 'app logs` Show input query variables in output for function runs~~ [ShopifyVM] 'app logs` Show input query variables in output for FunctionRunLog Oct 3, 2024

mssalemi changed the title ~~[ShopifyVM] 'app logs` Show input query variables in output for FunctionRunLog~~ [ShopifyVM] 'app logs' Show input query variables in output for FunctionRunLog Oct 3, 2024

mssalemi force-pushed the ms.app-logs-add-iqv-output branch 3 times, most recently from d104c32 to ef0df79 Compare October 3, 2024 14:52

mssalemi requested review from DuncanUszkay1, lopert and nickwesselman October 3, 2024 14:54

mssalemi commented Oct 3, 2024

View reviewed changes

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

andrewhassan reviewed Oct 8, 2024

View reviewed changes

DuncanUszkay1 reviewed Oct 8, 2024

View reviewed changes

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

DuncanUszkay1 approved these changes Oct 8, 2024

View reviewed changes

mssalemi requested a review from andrewhassan October 9, 2024 12:24

andrewhassan reviewed Oct 9, 2024

View reviewed changes

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

packages/app/src/cli/services/app-logs/utils.ts Outdated Show resolved Hide resolved

packages/app/src/cli/services/app-logs/utils.ts Outdated Show resolved Hide resolved

mssalemi requested a review from a team as a code owner October 9, 2024 16:15

mssalemi requested review from jamieguerrero and jantnovi and removed request for a team October 9, 2024 16:15

andrewhassan approved these changes Oct 9, 2024

View reviewed changes

mssalemi force-pushed the ms.app-logs-add-iqv-output branch 4 times, most recently from 47ba46e to 86135c0 Compare October 10, 2024 13:38

mssalemi enabled auto-merge October 15, 2024 12:15

add iqv sections to app logs command, refactor for individual tests

84e2ef1

mssalemi force-pushed the ms.app-logs-add-iqv-output branch from 86135c0 to 84e2ef1 Compare October 17, 2024 18:07

jamieguerrero reviewed Oct 17, 2024

View reviewed changes

code review updates, convert function log input query variables to ob…

b318ad7

…ject

nickwesselman approved these changes Oct 17, 2024

View reviewed changes

mssalemi requested a review from jamieguerrero October 18, 2024 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ShopifyVM] 'app logs' Show input query variables in output for FunctionRunLog #4566

[ShopifyVM] 'app logs' Show input query variables in output for FunctionRunLog #4566

mssalemi commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024

andrewhassan Oct 8, 2024

DuncanUszkay1 left a comment

andrewhassan left a comment

mssalemi commented Oct 17, 2024

nickwesselman commented Oct 17, 2024

mssalemi commented Oct 17, 2024 •

edited

Loading

nickwesselman commented Oct 17, 2024

jamieguerrero Oct 17, 2024

mssalemi Oct 17, 2024

jamieguerrero Oct 17, 2024

mssalemi Oct 17, 2024

mssalemi Oct 18, 2024

mssalemi commented Oct 17, 2024

[ShopifyVM] 'app logs' Show input query variables in output for FunctionRunLog #4566

Are you sure you want to change the base?

[ShopifyVM] 'app logs' Show input query variables in output for FunctionRunLog #4566

Conversation

mssalemi commented Oct 2, 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 Oct 2, 2024

github-actions bot commented Oct 3, 2024 • edited Loading

Coverage report

Test suite run success

github-actions bot commented Oct 3, 2024

andrewhassan Oct 8, 2024

Choose a reason for hiding this comment

DuncanUszkay1 left a comment

Choose a reason for hiding this comment

andrewhassan left a comment

Choose a reason for hiding this comment

mssalemi commented Oct 17, 2024

nickwesselman commented Oct 17, 2024

mssalemi commented Oct 17, 2024 • edited Loading

nickwesselman commented Oct 17, 2024

jamieguerrero Oct 17, 2024

Choose a reason for hiding this comment

mssalemi Oct 17, 2024

Choose a reason for hiding this comment

jamieguerrero Oct 17, 2024

Choose a reason for hiding this comment

mssalemi Oct 17, 2024

Choose a reason for hiding this comment

mssalemi Oct 18, 2024

Choose a reason for hiding this comment

mssalemi commented Oct 17, 2024

mssalemi commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024 •

edited

Loading

mssalemi commented Oct 17, 2024 •

edited

Loading