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

When using SLS_DEBUG env var, more log will be dumped #1803

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

kennyhyun
Copy link
Contributor

@kennyhyun kennyhyun commented May 23, 2024

What did you implement:

Related to #1508 and probably #1653 too

How did you implement it:

How can we verify it:

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@kennyhyun kennyhyun force-pushed the fix/utils-error-logging branch from 445e35d to df79b4c Compare May 23, 2024 05:57
@vicary vicary requested a review from j0k3r May 23, 2024 06:26
@vicary
Copy link
Member

vicary commented May 23, 2024

@kennyhyun What was the errors you are working with?

Depending on the specific error that is sent to stdout instead of stderr, it could either be

  1. An incorrect implementation, or
  2. Changing toString() in SpawnError may be a better option instead.

@j0k3r
Copy link
Member

j0k3r commented May 23, 2024

@vicary I think it's related to #1508 (comment)

@kennyhyun
Copy link
Contributor Author

kennyhyun commented May 23, 2024

yes @j0k3r thanks

@vicary yeah I tried SpawnError as well but it was not logged.

and SpawnError is an exported function, so I could not imagine the consequences.

if it logs errors, and you guys think SpawnError is better, I can change

Thanks for the quick response!

@kennyhyun
Copy link
Contributor Author

kennyhyun commented May 23, 2024

after reading more, I think npm/yarn packager doesn't log stdout from the SpawnError instance

should I merge stdout into stderr if stderr was empty string or packagers should be updated? I am two minded


probably keep this for now

@kennyhyun
Copy link
Contributor Author

kennyhyun commented May 23, 2024

@vicary one of the errors I was dealing with was,

.../@nodert-win10-rs4/windows.storage install$ node-gyp rebuild
../../../../../node_modules/parse-domain postinstall$ run-s build:tries
.../@nodert-win10-rs4/windows.storage install: gyp info it worked if it ends with ok
.../@nodert-win10-rs4/windows.storage install: gyp info using [email protected]
.../@nodert-win10-rs4/windows.storage install: gyp info using [email protected] | linux | x64
.../@nodert-win10-rs4/windows.storage install: gyp info find Python using Python version 3.11.2 found at "/usr/bin/python3"
.../@nodert-win10-rs4/windows.storage install: (node:103869) [DEP0150] DeprecationWarning: Setting process.config is deprecated. In the future the property will be read-only.
.../@nodert-win10-rs4/windows.storage install: (Use `node --trace-deprecation ...` to show where the warning was created)
.../@nodert-win10-rs4/windows.storage install: gyp info spawn /usr/bin/python3
.../@nodert-win10-rs4/windows.storage install: gyp info spawn args [

which resolved by downgrading python to 3.10

I could log this from stdout, I guess it was from npm, but am sure yarn was the same.

@kennyhyun
Copy link
Contributor Author

Sorry I cannot reproduce that anymore.

Can I just console.log stdout when SLS_DEBUG was specified instead?

@kennyhyun kennyhyun marked this pull request as ready for review May 23, 2024 11:59
@vicary
Copy link
Member

vicary commented May 24, 2024

@j0k3r thanks for referencing to the source!

@kennyhyun It's not easy to generalize a solution at the level of spawnProcess. But when we look at npm.js and yarn.js, it's more safer to do stdout + stderr at the respective .catch(). What do you think?

@kennyhyun
Copy link
Contributor Author

@vicary thanks for the feedback.

yeah I agree, let me do that in npm.js and yarn.js

@kennyhyun kennyhyun force-pushed the fix/utils-error-logging branch from eab7768 to 32a6b9f Compare May 24, 2024 15:55
Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Looks great, you can squash 👍

@j0k3r j0k3r requested a review from vicary June 14, 2024 09:51
@j0k3r j0k3r force-pushed the fix/utils-error-logging branch from edf1d95 to 8539948 Compare June 18, 2024 07:08
@j0k3r j0k3r enabled auto-merge June 18, 2024 07:08
@j0k3r j0k3r changed the title Fix/utils error logging When using SLS_DEBUG en var, more log will be dumped Jun 18, 2024
@j0k3r j0k3r changed the title When using SLS_DEBUG en var, more log will be dumped When using SLS_DEBUG env var, more log will be dumped Jun 18, 2024
@j0k3r j0k3r merged commit 984d359 into serverless-heaven:master Jun 18, 2024
13 checks passed
@j0k3r j0k3r added this to the 5.14.1 milestone Jun 18, 2024
@j0k3r j0k3r mentioned this pull request Jun 18, 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.

3 participants