[Themes] Add status codes and event methods to user logging #4605

EvilGenius13 · 2024-10-08T19:20:23Z

WHY are these changes introduced?

Iterating on the last user logging improvements

Follow up to: #4536
Closes: https://github.com/Shopify/develop-advanced-edits/issues/359

WHAT is this pull request doing?

This PR adds more functionality to user logging when using the Dev Server.

Highlights:

Pulled the logRequestLine function into it's own util file for use in other areas
Added logging to proxy requests allowing us to see POST requests (not including /cdn routes)
Added some colour distinction to the output such as status codes and response time

Output example with changes:

How to test your changes?

Pull down the branch
Build the branch
run theme dev
click around and watch the logs

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-08T19:24:43Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.73% (+0.24% 🔼)	8563/11773
🟡	Branches	69.71% (+0.57% 🔼)	4204/6031
🟡	Functions	71.7% (-0.06% 🔻)	2212/3085
🟡	Lines	73.06% (+0.27% 🔼)	8105/11093

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / function-upload-url-generate.ts	100%	100%	100%	100%
🟢	`...` / authorize.ts	100%	75%	100%	100%
🔴	`...` / post-auth.ts	44.12%	0%	0%	45.45%
🔴	`...` / redirect-listener.ts	14.75%	0%	33.33%	15%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / app.test-data.ts	91.4%	91.09% (-0.09% 🔻)	81.01%	90.8%
🟢	`...` / app.ts	87.07%	70.89% (-0.72% 🔻)	92%	88.37%
🟢	`...` / function.ts	86.36% (-0.59% 🔻)	86.36%	83.33%	86.36% (-0.59% 🔻)
🟢	`...` / app-context.ts	100%	87.5% (-1.97% 🔻)	100%	100%
🟢	`...` / deploy.ts	87.18%	85% (-7.86% 🔻)	87.5%	89.19%
🟢	`...` / generate.ts	100%	80% (-2.14% 🔻)	100%	100%
🟢	`...` / versions-list.ts	100%	85.71% (-1.79% 🔻)	100%	100%
🟢	`...` / show.ts	100%	66.67% (-8.33% 🔻)	100%	100%
🔴	`...` / extension.ts	55.26% (-0.29% 🔻)	50%	57.14%	56.76% (-0.06% 🔻)
🟡	`...` / update-extension.ts	66.67% (-3.33% 🔻)	54.55% (-3.79% 🔻)	60%	70.97% (-3.32% 🔻)
🟡	`...` / build.ts	74.49%	59.09% (-2.27% 🔻)	75.76%	72.22%
🟢	`...` / extension.ts	91.4% (+0.09% 🔼)	73.58%	91.3% (-0.36% 🔻)	91.21% (+0.1% 🔼)
🔴	`...` / app-management-client.ts	20.75% (-0.09% 🔻)	10.26%	22.58% (-0.25% 🔻)	19% (-0.09% 🔻)
🔴	`...` / partners-client.ts	26.87% (-0.2% 🔻)	40%	18.18% (-0.34% 🔻)	26.56% (-0.21% 🔻)
🟢	`...` / session.ts	81.01% (-1.99% 🔻)	72.32% (-0.41% 🔻)	92.86%	80.67% (-2.09% 🔻)
🟡	`...` / fs.ts	62.5%	84.62% (-7.05% 🔻)	58.97%	62.5%
🟢	`...` / admin.ts	82.76% (+1.51% 🔼)	37.5% (-2.5% 🔻)	90%	85.71% (+1.84% 🔼)

Test suite run success

1948 tests passing in 877 suites.

Report generated by 🧪jest coverage report action from 581abd8

EvilGenius13 · 2024-10-08T19:25:24Z

For status codes I went with green for any 2xx status, yellow for 3xx (redirects), and red for anything 4xx-5xx.

I'm also torn on part of the logging. On every request we make a /checkouts/internal/preloads.js? request as well. This adds some noise to the output. I considered ignoring it but I'm not sure if that would cause a debugging issue if someone wanted to make sure that was working as intended. Happy for some opinions.

@frandiox I would love your feedback based on your original comments from the PR prior to this. Would you want padding for the Status codes to always be in line or does this seem sufficient.

packages/theme/src/cli/utilities/log-request-line.ts

packages/theme/src/cli/utilities/theme-environment/proxy.ts

frandiox

Thanks for the changes!

I think I would personally prefer aligning them to the right:

Request >>  GET 200 /...
Request >>  GET 404 /...
Request >> POST 200 /...
Request >>  GET 200 /...
Request >> POST 400 /...

If we want we could do the same with Request and Synced. Also, to make them more similar, another possibility is moving the statuses after the path:

Request >>    GET  /...  200 400ms
Request >>   POST  /...  200  500ms
 Synced >> update  /layout/theme...
Request >>    GET  /...  404  150ms

Just a random thought, not sure if this looks any better.
cc @jamesmengo I feel you probably have a better judgement on this 😂

packages/theme/src/cli/utilities/theme-environment/proxy.ts

EvilGenius13 · 2024-10-09T18:28:21Z

Thanks for the changes!

I think I would personally prefer aligning them to the right:
Request >>  GET 200 /...
Request >>  GET 404 /...
Request >> POST 200 /...
Request >>  GET 200 /...
Request >> POST 400 /...
If we want we could do the same with Request and Synced. Also, to make them more similar, another possibility is moving the statuses after the path:
Request >>    GET  /...  200 400ms
Request >>   POST  /...  200  500ms
 Synced >> update  /layout/theme...
Request >>    GET  /...  404  150ms
Just a random thought, not sure if this looks any better. cc @jamesmengo I feel you probably have a better judgement on this 😂

This is the current look which the changes

Although I prefer seeing the status code after the event method, I can see how it aligns better with the Synced >>

So the only piece left is that the actions are lower cases in Synced and I think that helps quickly differentiate what kind of thing we're looking at (Request vs Synced) if were browsing details.

If needed I can probably push the action over but that makes me wonder if I should uppercase the action.

frandiox

Thanks so much for all the updates! I'll defer to the other reviewers for the log styles 🙌

packages/theme/src/cli/utilities/theme-environment/proxy.ts

karreiro

Thank you for this PR, @EvilGenius13! Logs look so much better 🤩

I've left only one refactoring + unit testing suggestion, and with that we will be ready to merge :)

Although I prefer seeing the status code after the event method, I can see how it aligns better with the Synced >>

Loved the idea of aligning them to the right, but I strongly prefer the status codes after the event method as well 😅

Thanks again for this PR!

packages/theme/src/cli/utilities/theme-environment/proxy.ts

jamesmengo

🚀 Nice work and thanks for the changes!

+1 to adding the guard condition in logRequestLine + Guilherme's styling suggestions before merging

karreiro

Thank you, @EvilGenius13! 🚀

This commit moves the logRequestLine function into a separate file and adds status codes and event methods. It grabs both get and post requests

EvilGenius13 requested review from karreiro, frandiox and jamesmengo October 8, 2024 19:20

This comment has been minimized.

Sign in to view

jamesmengo reviewed Oct 8, 2024

View reviewed changes

packages/theme/src/cli/utilities/log-request-line.ts Outdated Show resolved Hide resolved

jamesmengo reviewed Oct 8, 2024

View reviewed changes

packages/theme/src/cli/utilities/log-request-line.ts Outdated Show resolved Hide resolved

jamesmengo reviewed Oct 8, 2024

View reviewed changes

packages/theme/src/cli/utilities/theme-environment/proxy.ts Outdated Show resolved Hide resolved

frandiox reviewed Oct 9, 2024

View reviewed changes

packages/theme/src/cli/utilities/theme-environment/proxy.ts Outdated Show resolved Hide resolved

EvilGenius13 force-pushed the log-improvment-iteration branch from a6a43d3 to 7b84b2d Compare October 9, 2024 18:04

EvilGenius13 requested a review from a team as a code owner October 9, 2024 18:04

EvilGenius13 requested review from jantnovi and gonzaloriestra and removed request for a team October 9, 2024 18:04

EvilGenius13 force-pushed the log-improvment-iteration branch from 7b84b2d to 4e3e92c Compare October 9, 2024 18:28

EvilGenius13 requested review from jamesmengo and frandiox October 9, 2024 18:28

frandiox approved these changes Oct 10, 2024

View reviewed changes

packages/theme/src/cli/utilities/theme-environment/proxy.ts Outdated Show resolved Hide resolved

karreiro reviewed Oct 10, 2024

View reviewed changes

packages/theme/src/cli/utilities/theme-environment/proxy.ts Outdated Show resolved Hide resolved

jamesmengo approved these changes Oct 10, 2024

View reviewed changes

EvilGenius13 force-pushed the log-improvment-iteration branch 3 times, most recently from b2e18d1 to 2fb5be6 Compare October 11, 2024 16:36

EvilGenius13 requested a review from karreiro October 11, 2024 16:36

karreiro approved these changes Oct 15, 2024

View reviewed changes

EvilGenius13 enabled auto-merge October 15, 2024 13:42

add status codes and event methods to log requests

581abd8

This commit moves the logRequestLine function into a separate file and adds status codes and event methods. It grabs both get and post requests

EvilGenius13 force-pushed the log-improvment-iteration branch from 2fb5be6 to 581abd8 Compare October 15, 2024 17:28

karreiro removed request for jantnovi and gonzaloriestra October 16, 2024 17:20

karreiro disabled auto-merge October 16, 2024 17:22

karreiro enabled auto-merge October 16, 2024 17:22

karreiro disabled auto-merge October 17, 2024 05:09

karreiro added this pull request to the merge queue Oct 17, 2024

Merged via the queue into main with commit b0c806b Oct 17, 2024
36 checks passed

karreiro deleted the log-improvment-iteration branch October 17, 2024 05:16

shopify-shipit bot temporarily deployed to nightly October 17, 2024 09:31 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Themes] Add status codes and event methods to user logging #4605

[Themes] Add status codes and event methods to user logging #4605

EvilGenius13 commented Oct 8, 2024 •

edited

Loading

This comment has been minimized.

github-actions bot commented Oct 8, 2024 •

edited

Loading

EvilGenius13 commented Oct 8, 2024

frandiox left a comment

EvilGenius13 commented Oct 9, 2024

frandiox left a comment

karreiro left a comment •

edited

Loading

jamesmengo left a comment

karreiro left a comment

[Themes] Add status codes and event methods to user logging #4605

[Themes] Add status codes and event methods to user logging #4605

Conversation

EvilGenius13 commented Oct 8, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

This comment has been minimized.

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

Coverage report

Test suite run success

EvilGenius13 commented Oct 8, 2024

frandiox left a comment

Choose a reason for hiding this comment

EvilGenius13 commented Oct 9, 2024

frandiox left a comment

Choose a reason for hiding this comment

karreiro left a comment • edited Loading

Choose a reason for hiding this comment

jamesmengo left a comment

Choose a reason for hiding this comment

karreiro left a comment

Choose a reason for hiding this comment

EvilGenius13 commented Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 8, 2024 •

edited

Loading

karreiro left a comment •

edited

Loading