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: Bind log functions to sanitized context #27 #29

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SwapnilBhosale
Copy link

@SwapnilBhosale SwapnilBhosale commented Feb 13, 2019

Closes #27

Fix for issue 27 support for log, info,error, warn and verbose methods of context objects

  1. Do not add explicit handling for log function in sanitizeContext method
  2. remove log related test cases
  3. Tested on Azure function and now able to log with all the levels specified

Following are the screenshot :
image

@codecov-io
Copy link

codecov-io commented Feb 13, 2019

Codecov Report

Merging #29 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #29   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          67     67           
  Branches       18     18           
=====================================
  Hits           67     67
Impacted Files Coverage Δ
src/IncomingMessage.js 100% <ø> (ø) ⬆️

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 2bd9041...76a8b64. Read the comment docs.

Copy link
Owner

@yvele yvele left a comment

Choose a reason for hiding this comment

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

Please don't change the space formatting everywhere..
This also makes the PR very hard to review

Only make changes on what's needed

@yvele yvele changed the title PR for issue #27 fix: Bind log functions to sanitized context #27 Feb 13, 2019
@yvele yvele self-assigned this Feb 13, 2019
@SwapnilBhosale
Copy link
Author

Please don't change the space formatting everywhere..
This also makes the PR very hard to review

Only make changes on what's needed

  • Corrected formatting as it was before.
  • Change the stuff which was only needed

@yvele
Copy link
Owner

yvele commented Feb 14, 2019

So basically the log function can be passed destructured, and don't need to be bound using bind 🤔

Is this compatible will all Azure Function runtimes? What Azure Function version are you using?

I'm also thinking.. in a further commit, to pass contextas it without sanitizing it

@yvele yvele added the bug label Feb 14, 2019
@SwapnilBhosale
Copy link
Author

SwapnilBhosale commented Feb 14, 2019

So basically the log function can be passed destructured, and don't need to be bound using bind 🤔

Is this compatible will all Azure Function runtimes? What Azure Function version are you using?

I'm also thinking.. in a further commit, to pass contextas it without sanitizing it

I think, Yes, you do not have to bind the log function.
Please check the screenshot atatched for issue #27 .
I have tested on azure function v~1.
Will update you for v2 in some time

@SwapnilBhosale
Copy link
Author

SwapnilBhosale commented Feb 14, 2019

So basically the log function can be passed destructured, and don't need to be bound using bind 🤔
Is this compatible will all Azure Function runtimes? What Azure Function version are you using?
I'm also thinking.. in a further commit, to pass contextas it without sanitizing it

I think, Yes, you do not have to bind the log function.
Please check the screenshot atatched for issue #27 .
I have tested on azure function v~1.
Will update you for v2 in some time

Tested on V2. It is also working as expected. Seems, nothing is broken with the fix
image

image

@SwapnilBhosale
Copy link
Author

SwapnilBhosale commented Feb 14, 2019

Dear Yvele,

We are using this library in production, and we are blocked because of this issue(without context.log.error support we can not monitor azure function for any errors or downtime as in appinsight we look for errors).

Highly appreciate if you can release a minor version asap.

Copy link
Author

@SwapnilBhosale SwapnilBhosale left a comment

Choose a reason for hiding this comment

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

Incorporated formatting changes

@xXanth0s
Copy link

So what is the status here?
Is the repository dead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error, warn, info, and verbose log functions are not bind to the sanitized context
4 participants