Skip to content

Conversation

@freelerobot
Copy link
Contributor

@freelerobot freelerobot commented Nov 10, 2020

Fixes #929 🦕

According to specs and docs, users should be able to log httpRequest type logs independently of a log message.

In this case, the log message (jsonPayload field) can just show up as {}. And of course, jsonPayload field should not contain the httpRequest data.

Note:

  • Users can now log entry(metadata.httprequest, nil) and jsonPayload will show up as {}
  • Users can still log entry(nil, message)
  • Users can still log entry(metadata, message) - regular case
  • See test cases

Solves customer issue: b/172339240

@freelerobot freelerobot requested review from a team as code owners November 10, 2020 23:30
@freelerobot freelerobot self-assigned this Nov 10, 2020
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/nodejs-logging API. label Nov 10, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 10, 2020
@freelerobot freelerobot added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Nov 10, 2020
@codecov
Copy link

codecov bot commented Nov 10, 2020

Codecov Report

Merging #942 (10b3433) into master (93be851) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #942   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files          18       18           
  Lines       12971    12977    +6     
  Branches      411      412    +1     
=======================================
+ Hits        12762    12768    +6     
  Misses        207      207           
  Partials        2        2           
Impacted Files Coverage Δ
src/log.ts 99.80% <100.00%> (+<0.01%) ⬆️

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 93be851...10b3433. Read the comment docs.

@freelerobot freelerobot changed the title feat!: users can log httprequest logs without providing a log message fix!: users can log httprequest logs without providing a log message Nov 10, 2020
@freelerobot freelerobot requested a review from bcoe November 12, 2020 16:35
@bcoe
Copy link

bcoe commented Nov 16, 2020

@nicoleczhu this seems like the right fix to me 👍

The only thing we should be mindful of, is that we try to keep breaking changes to once every year or so. I think we could choose to do a breaking change of the library now, but it might be good to see if there are any other breaking changes we want to make at the same time.

@freelerobot
Copy link
Contributor Author

@nicoleczhu this seems like the right fix to me 👍

The only thing we should be mindful of, is that we try to keep breaking changes to once every year or so. I think we could choose to do a breaking change of the library now, but it might be good to see if there are any other breaking changes we want to make at the same time.

Makes sense! I was hesitant to release this for that very reason. I'm gonna work on a few more PRs this month, and see if we can bundle a breaking update containing multiple new features/fixes. Tagging this one as do not merge for now.

@freelerobot freelerobot added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 16, 2020
@freelerobot freelerobot removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 25, 2020
Copy link

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Could we squint, and not call this a breaking change, since I believe we're making it work as intended?

Someone setting httpRequest would expect this meta information to be handled differently right?

@freelerobot freelerobot changed the title fix!: users can log httprequest logs without providing a log message fix: users can log httprequest logs without providing a log message Nov 30, 2020
@freelerobot
Copy link
Contributor Author

Could we squint, and not call this a breaking change, since I believe we're making it work as intended?

Someone setting httpRequest would expect this meta information to be handled differently right?

I think most new users would expect this fix as the default behavior.

It will only be breaking for users who were aware of the bug on our end && were custom handling the poorly structured logs themselves.

@bcoe
Copy link

bcoe commented Nov 30, 2020

@nicoleczhu it's okay for us to release a major of logging, if you think this is likely to bite people.

@freelerobot freelerobot changed the title fix: users can log httprequest logs without providing a log message fix!: users can log httprequest logs without providing a log message Nov 30, 2020
@freelerobot freelerobot merged commit d72a296 into master Dec 1, 2020
@release-please release-please bot mentioned this pull request Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: logging Issues related to the googleapis/nodejs-logging API. cla: yes This human has signed the Contributor License Agreement. priority: p2 Moderately-important priority. Fix may not be included in next release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

entry: httprequest without a log message should format correctly

4 participants