Add two new flags to enable HTTP Server and clients logs #166
Conversation
Codecov Report
@@ Coverage Diff @@
## release-candidate #166 +/- ##
===================================================
Coverage 99.83% 99.83%
===================================================
Files 164 164
Lines 12977 13147 +170
===================================================
+ Hits 12955 13125 +170
Misses 22 22
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jvanecek
force-pushed
the
zinc-logging
branch
from
9e7afc6
to
e920cd5
Compare
jvanecek
changed the title
|
|
||
| ZnLogEvent announcer | ||
| when: ZnServerHandlerErrorEvent | ||
| do: [ :event | LaunchpadLogRecord emitError: event exception messageText ] |
There was a problem hiding this comment.
We can send directly a message to the event here, something like emitAsLogRecord, so we don't need to expose the internals of the event.
There was a problem hiding this comment.
And we can test more easily different combinations of event data
| Extension { #name : #NeoJSONWriter } | ||
|
|
||
| { #category : #'*Stargate-Model' } | ||
| NeoJSONWriter >> mapZincEvent [ |
There was a problem hiding this comment.
I think this needs better tests, we need to be sure that every kind of object the event includes is properly mapped (like the abstractions we have in Hyperspace: WebLink, EntityTags, etc).
| mapper | ||
| mapAccessor: #method; | ||
| mapProperty: #uri getter: [ :request | request uri printString ]; | ||
| mapProperty: #body getter: [ :request | request contents ]; |
There was a problem hiding this comment.
We need to be careful here, I don't know if it's a good idea to just toss the contents of the request (a user-controlled thing) directly to the mapper. Probably we need to do something different for text and binary bodies and put an upper limit to the bytes to log.
There was a problem hiding this comment.
I agree. We need to think about the use cases and our broader context. I think that we can rely on a external component to do a detailed log about every request (too expensive at a smalltalk image level). Maybe we need just the URI and method.
| (response hasEntity and: [ response entity hasContentType ]) | ||
| then: [ response contents asString copyNoMoreThanFirst: 4000 ] | ||
| otherwise: [ nil ] ]; |
There was a problem hiding this comment.
Same here, we need to be extra careful with the kind of content we handle here. I think we need to make a distinction between textual and binary content.
| { | ||
| ZnServerTransactionEvent. | ||
| ZnSimplifiedServerTransactionEvent. | ||
| ZnClientTransactionEvent } do: [ :event | |
There was a problem hiding this comment.
event here can be eventType and this makes event available for the inner temp
|
|
||
| self for: ZnHeaders customDo: [ :mapping | | ||
| mapping writer: [ :jsonWriter :headers | | ||
| jsonWriter writeMap: headers ] ]. |
There was a problem hiding this comment.
Some headers will have sensitive data (like the Authorization ones), so we need to mask these ones to not expose sensitive information in the logs.
| | eventJson | | ||
| eventJson := NeoJSONObject fromString: | ||
| (String streamContents: [ :stream | | ||
| (NeoJSONWriter on: stream) | ||
| mapZincEvent; | ||
| nextPut: aZincEvent ]). | ||
| aLogRecordJson | ||
| at: #summary put: eventJson summary; | ||
| at: #request put: eventJson request; | ||
| at: #response put: eventJson response | ||
| ] |
There was a problem hiding this comment.
We have to consider the possibility of something that can be mapped here, and producing an error log record in this case, but not killing the app because some log records cannot be produced.
No description provided.