Json logging #836

arianvp · 2019-08-23T15:12:09Z

No description provided.

arianvp · 2019-08-23T16:20:45Z

Self-review:

Use of this type is discouraged. Note the following equivalence:

Data.Monoid.Last x === Maybe (Data.Semigroup.Last x)

In addition to being equivalent in the structural sense, the two
also have 'Monoid' instances that behave the same. This type will
be marked deprecated in GHC 8.8, and removed in GHC 8.10.
Users are advised to use the variant from "Data.Semigroup" and wrap
it in 'Maybe'.

fisx · 2019-08-26T14:44:51Z

libs/imports/src/Orphans.hs

@@ -6,10 +6,3 @@
 -- | Orphan instances for non-Wire-specific types and classes.
 module Orphans () where


please decomission the orphanage. it's abandoned.

The old netstrings option is now deprecated, use logFormat instead. logFormat takes presedence of netstrings if both are set. If none are set, log format defaults to Plain

The implementation is very trivial at the moment. Namely "undefined" as I think we should spend some time bikeshedding what the log format will look like in JSON. Once we have done that I can implement it in a few minutes

arianvp · 2019-08-27T13:00:29Z

@fisx Example logs:

[galley] {"request":"N/A","targets":"336a018e-c8f1-49c4-b395-f781c94a255f","msgs":["D","Teams.addTeamMemberInternal"]}
[galley] {"code":"403","label":"legalhold-not-enabled","request":"N/A","msgs":["D","\"legal hold is not enabled for this team\""]
[galley] {"code":"400","label":"legalhold-invalid-key","request":"N/A","msgs":["D","\"legal hold service pubkey is invalid\""]}
[galley] {"request":"N/A","targets":"2a18c226-eb2f-4037-8d74-dac55a04417c","msgs":["D","LegalHold.approveDevice"]}
[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings"]}
[galley] {"code":"403","label":"no-team-member","request":"N/A","msgs":["D","\"Requesting user is not a team member.\""]}
[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings"]}
[galley] {"code":"403","label":"operation-denied","request":"N/A","msgs":["D","\"Insufficient permissions (missing ChangeLegalHoldTeamSettings)\""]}
[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings"]}
[galley] {"code":"403","label":"access-denied","request":"N/A","msgs":["D","\"This operation requires reauthentication\""]}
[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings"]}
[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings'"]}
[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings"]}
[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings'"]}
[galley] {"code":"404","label":"no-team","request":"N/A","msgs":["D","\"team not found\""]}
[galley] {"code":"404","label":"no-team","request":"N/A","msgs":["D","\"team not found\""]}
[galley] {"request":"N/A","targets":"e5fc30a4-a24b-424b-a827-4ba1ee566882","msgs":["D","Teams.addTeamMemberInternal"]}
[galley] {"request":"N/A","targets":"[\"992eb7f9-2c28-4172-97b0-b44b9e5a4119\",\"e5fc30a4-a24b-424b-a827-4ba1ee566882\"]","msgs":["D","LegalHold.createSettings"]}
[galley] {"request":"N/A","targets":"e5fc30a4-a24b-424b-a827-4ba1ee566882","msgs":["D","LegalHold.requestDevice"]}

tiago-loureiro · 2019-08-27T13:18:53Z

[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings"]}

The \" is due to the show . toByteString mentioned here

Also, I see that "D" shows up as "msgs", that should be a diff. field named "level"

arianvp · 2019-08-27T16:25:16Z

Also, I see that "D" shows up as "msgs", that should be a diff. field named "level"

This I cannot change I think. tinylog internally sets the loglevel not as a field but just appends it to the Msg I think. I'm not sure how to not make it do that? I'm actually ignoring the logLevel argument in my jsonRenderer function https://github.com/wireapp/wire-server/pull/836/files#diff-79a662d6500cd50cb5717cf50673a5c8R62 so I hoped this would cause the log level to not show up in the msgs field

fisx · 2019-08-27T20:24:51Z

[galley] {"request":"N/A","targets":"[\"2a18c226-eb2f-4037-8d74-dac55a04417c\",\"65a325d3-5ec9-4669-97be-7605b76894db\"]","msgs":["D","LegalHold.removeSettings"]}

the targets value could be an array instead of a string. (not sure that makes sense; only do it if you like the idea, and it's easy to change.)

[debug level should have its own field]

there are ways to parse this out of the messages list again, but i think it's not worth the effort and the risk of complications with tinylog updates.

fisx

I feel very nit-picky now (sorry!), please let me know if I'm being annoying. The substance of your code is great. If you disagree with any of my comments, fight me! :-)

libs/extended/src/System/Logger/Extended.hs

services/galley/galley.integration.yaml

No need to declare an instance because encoding is only used locally. Furthermore, we avoid an intermediate list being allocated when serializing fields of a log message

arianvp · 2019-08-28T09:17:50Z

the targets value could be an array instead of a string. (not sure that makes sense; only do it if you like the idea, and it's easy to change.)

it's not easy to change. type Field = (Builder, Builder) Tinylog makes too many assumptions about the logformat for us to do something more sensible here

arianvp · 2019-08-28T09:20:01Z

Also, I see that "D" shows up as "msgs", that should be a diff. field named "level"

@tiago-loureiro do you want me to add an extra field called level eventhough I can not get rid of the D in the log messages? (I really still don't understand why this D is showing up there in the first place. I'm very confused about it)

fisx · 2019-08-28T09:30:28Z

the targets value could be an array instead of a string. (not sure that makes sense; only do it if you like the idea, and it's easy to change.)

it's not easy to change. type Field = (Builder, Builder) Tinylog makes too many assumptions about the logformat for us to do something more sensible here

you could parse it back from the string, i guess... :-) (could you?)

fisx · 2019-08-28T09:37:21Z

Also, I see that "D" shows up as "msgs", that should be a diff. field named "level"

@tiago-loureiro do you want me to add an extra field called level eventhough I can not get rid of the D in the log messages? (I really still don't understand why this D is showing up there in the first place. I'm very confused about it)

it's the call to lmsg in this line.

I say we leave it in and add an extra field for debug level as tiago suggested, just in case it'll make analytics easier.

fisx

I'm happy now! (Please do not merge before ci is happy, too! :))

libs/extended/src/System/Logger/Extended.hs

arianvp · 2019-08-28T09:50:02Z

you could parse it back from the string, i guess... :-) (could you?)

There is an unsafeToEncoding function that turns any bytestring into an Encoding. We could use that I guess but we have to be sure that what we put in is valid JSON otherwise bad things happen =)

* Add logs for team operations * Add logging for legalhold * Json logging (#836) * Allow to set log format The old netstrings option is now deprecated, use logFormat instead. logFormat takes presedence of netstrings if both are set. If none are set, log format defaults to Plain * Implement json renderer for tinylog

* Add json logging and logging (#828) (#836) * Add logs for team operations * Add logging for legalhold * Json logging (#836) * Allow to set log format The old netstrings option is now deprecated, use logFormat instead. logFormat takes presedence of netstrings if both are set. If none are set, log format defaults to Plain * Implement json renderer for tinylog * Specialize the error cases on conversation lookup. (#841) Getting a conversation you left will now give 403 access-denied, not 404 not-found. * Feature Flags in galley options. (#825) * Feature flags in galley options file. * Honour sso feature flag. * Honour legalhold feature flag (test). * Honour legalhold feature flag (handler). * Disable all LH tests if feature is disabled. * Check options settings for feature flag. * Avoid catch-all pattern. Catch-all patterns are not future-proof if constructors for new falvors of "disabled" are added.

fisx mentioned this pull request Aug 26, 2019

Cleanup integration tests #839

Merged

arianvp marked this pull request as ready for review August 26, 2019 14:42

fisx reviewed Aug 26, 2019

View reviewed changes

arianvp added 4 commits August 27, 2019 14:47

Allow to set log format

09526a7

The old netstrings option is now deprecated, use logFormat instead. logFormat takes presedence of netstrings if both are set. If none are set, log format defaults to Plain

Add a jsonRenderer for logs

54b5361

The implementation is very trivial at the moment. Namely "undefined" as I think we should spend some time bikeshedding what the log format will look like in JSON. Once we have done that I can implement it in a few minutes

Implement json instance for tinylog

dbd35b2

Use Semigroup Last

878c379

arianvp force-pushed the json-logging branch from 8d77eca to 878c379 Compare August 27, 2019 12:48

arianvp requested a review from fisx August 27, 2019 13:02

Orphan orphans

a97af06

fisx requested changes Aug 27, 2019

View reviewed changes

arianvp added 3 commits August 28, 2019 11:09

Make encoding of Element' slightly faster and remove ToJSON instance

f09f719

No need to declare an instance because encoding is only used locally. Furthermore, we avoid an intermediate list being allocated when serializing fields of a log message

Resolve review remarks

9299252

Revert galley json logging

3a981af

arianvp requested a review from fisx August 28, 2019 09:18

fisx approved these changes Aug 28, 2019

View reviewed changes

libs/extended/src/System/Logger/Extended.hs Show resolved Hide resolved

Fix typo

6818137

arianvp merged commit dfabfec into more-debug-logging Aug 29, 2019

arianvp deleted the json-logging branch August 29, 2019 09:49

fisx mentioned this pull request Sep 3, 2019

release_2019_09_03 #843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Json logging #836

Json logging #836

arianvp commented Aug 23, 2019

arianvp commented Aug 23, 2019

fisx Aug 26, 2019

arianvp commented Aug 27, 2019 •

edited

Loading

tiago-loureiro commented Aug 27, 2019

arianvp commented Aug 27, 2019 •

edited

Loading

fisx commented Aug 27, 2019

fisx left a comment

arianvp commented Aug 28, 2019

arianvp commented Aug 28, 2019

fisx commented Aug 28, 2019

fisx commented Aug 28, 2019

fisx left a comment

arianvp commented Aug 28, 2019 •

edited

Loading

		@@ -6,10 +6,3 @@
		-- \| Orphan instances for non-Wire-specific types and classes.
		module Orphans () where

Json logging #836

Json logging #836

Conversation

arianvp commented Aug 23, 2019

arianvp commented Aug 23, 2019

fisx Aug 26, 2019

Choose a reason for hiding this comment

arianvp commented Aug 27, 2019 • edited Loading

tiago-loureiro commented Aug 27, 2019

arianvp commented Aug 27, 2019 • edited Loading

fisx commented Aug 27, 2019

fisx left a comment

Choose a reason for hiding this comment

arianvp commented Aug 28, 2019

arianvp commented Aug 28, 2019

fisx commented Aug 28, 2019

fisx commented Aug 28, 2019

fisx left a comment

Choose a reason for hiding this comment

arianvp commented Aug 28, 2019 • edited Loading

arianvp commented Aug 27, 2019 •

edited

Loading

arianvp commented Aug 27, 2019 •

edited

Loading

arianvp commented Aug 28, 2019 •

edited

Loading