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

fix: log to console using structured logging #780

Merged
merged 10 commits into from
Dec 20, 2021
Merged

Conversation

minherz
Copy link
Contributor

@minherz minherz commented Dec 10, 2021

Replace package console-log-level with @google-cloud/logging-min. Use LogSync class of the logging package to print structured logging to stdout to capture the correct severity level.
Also handle a case of printing multi-line log that is captured as multiple log entries.

Replaces 'console-log-level' with '@google-cloud/logging-min' to write logs to stdout.
Rolls back to accept a string message for Logger methods.
Replaces new line with its escape notation '\\n'.
Refactors single location that prints array to print a single string.
@minherz minherz requested a review from a team as a code owner December 10, 2021 07:03
@product-auto-label product-auto-label bot added the api: cloudprofiler Issues related to the googleapis/cloud-profiler-nodejs API. label Dec 10, 2021
@codecov
Copy link

codecov bot commented Dec 10, 2021

Codecov Report

Merging #780 (6bf182c) into main (ac83b29) will decrease coverage by 0.82%.
The diff coverage is 53.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #780      +/-   ##
==========================================
- Coverage   69.97%   69.15%   -0.83%     
==========================================
  Files           7        7              
  Lines        1209     1248      +39     
  Branches       57       58       +1     
==========================================
+ Hits          846      863      +17     
- Misses        362      384      +22     
  Partials        1        1              
Impacted Files Coverage Δ
src/index.ts 54.86% <0.00%> (-0.20%) ⬇️
src/logger.ts 67.05% <56.45%> (-18.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac83b29...6bf182c. Read the comment docs.

@minherz
Copy link
Contributor Author

minherz commented Dec 10, 2021

@bcoe can you help me to troubleshoot the reasons for check failures, please?

@minherz
Copy link
Contributor Author

minherz commented Dec 13, 2021

Test fails will be fixed with mapbox/node-pre-gyp#623

Copy link

@losalex losalex left a comment

Choose a reason for hiding this comment

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

Some comments inside

src/logger.ts Outdated Show resolved Hide resolved
src/logger.ts Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
Copy link

@losalex losalex left a comment

Choose a reason for hiding this comment

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

Left some comments, see if you agree with those

src/logger.ts Outdated Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
src/logger.ts Outdated Show resolved Hide resolved
minherz and others added 3 commits December 17, 2021 06:51
ensure to pass test to not accept node@10.4.0
add new line at the end of the package.json.
add constants for max and min logging levels.
add handling windows new line in the multiline log payload.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: cloudprofiler Issues related to the googleapis/cloud-profiler-nodejs API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants