Feature/sdl server statistics #150
Conversation
Hotfix 2.6.1
|
Preview this pull request for SDL Server Documentation: |
|
Preview this pull request for SDL Server: |
|
Preview this pull request for SDL Server Guides: |
|
Preview this pull request for SDL Server Documentation: |
|
Preview this pull request for SDL Server: |
|
Preview this pull request for SDL Server Guides: |
There was a problem hiding this comment.
@russjohnson09 Hey Russell, unfortunately I need to return this back for revisions. Feedback below.
- Please follow existing project design patterns when contributing to a project. For example, please refrain from using async/await and Promises since the established design pattern in this project is the use of callbacks.
- Try/catch blocks generally indicate an area of improvement or lack of confidence in how something behaves and should therefore be avoided.
- Please consistently use semicolons.
- Please follow existing project design patterns of curly bracket placements.
- Postgres database helper changes appear to replicate existing behavior and should likely be removed.
- Remove extra
console.log()lines and miscellaneous comments prior to opening a PR. - Please use consistent styles of quotes for string content.
- Please follow existing project design patterns for unit tests.
- Please follow existing project design patterns for method parameter spacing.
- Refrain from using setTimeout(), as this typically creates race conditions.
- Please follow existing project design patterns for indentation (4 spaces in lieu of a tab).
- Please follow existing project design patterns of functional design rather than object-oriented design.
- Please follow existing project design patterns for SQL statements (e.g. keyword capitalization, constraint definitions, indentation)
If you have any questions or ever run into something you're not quite sure of, feel free to ask me or Chris - we're here to help :)
… uneeded changes. code style updates.
|
Preview this pull request for SDL Server Documentation: |
|
Preview this pull request for SDL Server: |
|
Preview this pull request for SDL Server Guides: |
|
Preview this pull request for SDL Server Documentation: |
|
Preview this pull request for SDL Server Guides: |
|
Preview this pull request for SDL Server: |
There was a problem hiding this comment.
Returning for heavy revisions. Please take your time to ensure we're producing clean, quality code which matches the project's existing styles and patterns.
In addition to code-specific feedback, the reporting APIs (controllers), SQL, and helpers should be separated into their respective components and reside in app/v1/reporting/* to follow the structure of the rest of the project. This is especially true since the policy table structure the reporting logic relies on is considered "v1".
Many of the suggestions made are repetitive, and in some cases were observed multiple times but were not explicitly noted in each occurrence for the sake of time.
| @@ -7,6 +7,7 @@ const path = require('path'); | |||
| const config = require('../../settings'); //configuration module | |||
| const log = require(`../../custom/loggers/${config.loggerModule}/index.js`); | |||
| const db = require(`../../custom/databases/${config.dbModule}/index.js`)(log); //pass in the logger module that's loaded | |||
| const reportingService = require(`../../lib/reporting-service/index.js`)({db,expirationDays: config.reporting.expirationDays,trackingEnabled: config.reporting.enabled}); | |||
There was a problem hiding this comment.
Please reference the database library and config file within the reporting-service rather than passing them as parameters. See existing project files for precedent.
| @@ -4,6 +4,17 @@ const sql = require('./sql.js'); | |||
| const flow = app.locals.flow; | |||
| const async = require('async'); | |||
|
|
|||
| function getReport (req, res, next) { | |||
| if(req.query.id === null || req.query.id === undefined) { | |||
There was a problem hiding this comment.
| if(req.query.id === null || req.query.id === undefined) { | |
| if(!req.query.id) { |
| @@ -684,6 +705,9 @@ export default { | |||
| } | |||
| }, | |||
| created: function(){ | |||
| if (this.REPORTING_ENABLED) { | |||
There was a problem hiding this comment.
Recommend moving this into getApp to conveniently refresh stats whenever app data changes
| var expect = common.expect; | ||
| var endpoint = '/api/v1/module/report'; | ||
|
|
||
| common.get( |
| @@ -0,0 +1,147 @@ | |||
| let devices = { | |||
| "device1": { | |||
| header: { | ||
| values: headers, | ||
| align: 'center', | ||
| line: {width: 1, color: 'black'}, |
There was a problem hiding this comment.
Inconsistent formatting of object properties. Each property should be on its own line.
| family: family, | ||
| size: 10, | ||
| color: "#A9B3BD", | ||
| weight: "bold"} |
There was a problem hiding this comment.
Closing bracket at end of line.
| height: 100, | ||
| values: (function() { | ||
| if (options.isPercent) | ||
| { |
There was a problem hiding this comment.
Please follow existing code style patterns/standards. Keep opening brackets on same line as if-statement.
| values, | ||
| ] | ||
| } | ||
| else { |
There was a problem hiding this comment.
Check formatting of else to align with existing project patterns.
| } | ||
| })() , | ||
| align: "center", | ||
| line: {color: "black", width: 1}, |
There was a problem hiding this comment.
Object property design pattern.
|
Closing. If the need arises this can be revisited |
Fixes #132
This PR is ready for review.
Risk
This PR makes minor API changes. No breaking changes. Just the addition of two new endpoints for reporting.
/module/report GET/applications/report GETTesting Plan
For API and unit testing /test/unit/reporting/reporting.js, /test/api/v1/applications/report.js, and /test/api/v1/module/report.js were added to the main mocha test plan and are executed whenever
npm run testis used. Full e2e tests were also done using a local instance of sdl_core.For UI testing charts were visually checked after they were populated using the mocha tests.
Summary
Two new endpoints were added.
/module/report GET/applications/report GETThree tables were created.
device,app_usage, andpolicy_table_update_request. These are populated with new report data whenever available from core's policy table update request.Changelog
Breaking Changes
No breaking changes. When an production policy update request is made through an existing endpoint like
/production/policy POSTthe response is given before the additional reporting data is processed so it should not affect the speed of the response for a single request. Some load testing was done using /test/unit/reporting/reporting-load.js.Enhancements
Reporting data can tracked with environment variable REPORTING_ENABLED=true. The number of days tracked is defaulted to 30 and can be set using environment variable REPORTING_EXPIRATION_DAYS.
Reporting data that is tracked is can be viewed in the UI in the new reporting option on the main side nav and by viewing individual applications.
Bug Fixes
None
Tasks Remaining:
None
CLA