Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Don't log message data #1491

Closed
wants to merge 1 commit into from
Closed

Don't log message data #1491

wants to merge 1 commit into from

Conversation

mikaelm12
Copy link
Contributor

Issue: #1449
We don't want to log message content on the client.

@BrennanConroy
Copy link
Member

Pretty sure there are more locations that we need to change.

@davidfowl
Copy link
Member

Also tests?

@analogrelay
Copy link
Contributor

analogrelay commented Feb 26, 2018

Also tests?

We don't do a lot of logging testing. But I guess we could. The test I'd prefer most for this one is a test that runs a full client/server scenario (possibly in separate tests) using TestSink/TestLogger and then searches every log message for a unique string present in the payload to verify it is not there, rather than one that tests specific messages and is therefore brittle as we change messages.

@davidfowl
Copy link
Member

It comes down to understanding how important this change is. If another change regresses this I have extremely low confidence that we'll catch it in a code review.

@analogrelay
Copy link
Contributor

Exactly the purpose of the test I suggested ;)

@davidfowl
Copy link
Member

@anurse and @mikaelm12 can you talk to @anpete and @ajcvickers about how EF handles logging of parameter values?

@mikaelm12
Copy link
Contributor Author

mikaelm12 commented Mar 8, 2018

Not working on this atm.

@mikaelm12 mikaelm12 closed this Mar 8, 2018
@BrennanConroy BrennanConroy deleted the mikaelm12/LogMessages branch August 6, 2018 19:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants