MLIBZ-1802: Logs #126
MLIBZ-1802: Logs #126
Conversation
Codecov Report
@@ Coverage Diff @@
## master #126 +/- ##
==========================================
+ Coverage 90.51% 90.58% +0.07%
==========================================
Files 77 77
Lines 6461 6521 +60
Branches 1041 1043 +2
==========================================
+ Hits 5848 5907 +59
- Misses 613 614 +1
Continue to review full report at Codecov.
|
|
|
||
| toPlainObject() { | ||
| return { | ||
| id: this.id, |
There was a problem hiding this comment.
@thomasconner Is there any other value to the id outside of logging it here? Do we tie the request ID, for example, through the pipeline that the request goes through?
There was a problem hiding this comment.
This id is generated by the SDK and used to match up log statements for requests. Multiple requests might be logged at the same time and without some kind of identifier it would be hard to match log statements that are related. We need to communicate that this is not related to the X-Kinvey-Request-Id header by adding the information to the troubleshooting guide.
src/utils/src/log.js
Outdated
| } | ||
| }; | ||
| }; | ||
| log.setLevel(log.getLevel()); |
There was a problem hiding this comment.
@thomasconner this looks odd. This should be a No-Op if getters and setters do what they're supposed to :)
package.json
Outdated
| "local-storage": "1.4.2", | ||
| "lodash": "4.17.4", | ||
| "loglevel": "1.4.1", | ||
| "loglevel": "github:thomasconner/loglevel", |
There was a problem hiding this comment.
@thomasconner what were the changes you made to the package? Do you plan to keep your fork updated periodically from the master?
There was a problem hiding this comment.
I submitted a PR that was merged into the loglevel master branch. We can use the latest release of loglevel instead of a personally maintained version.
src/query.js
Outdated
| const json = this.toPlainObject(); | ||
| data = sift(json.filter, data); | ||
|
|
||
| Log.debug('After applying query filter to data', data); |
There was a problem hiding this comment.
@thomasconner Have you considered that this data could be quite large and flood the log? Would it make sense to truncate or just print the count instead, for example?
There was a problem hiding this comment.
Yes we probably should not log the actual data. I think we will just log the length of the data to begin.
Description
This PR adds logging to the SDK.
Changes