{"action":"created","issue":{"url":"https://api.github.com/repos/firehol/netdata/issues/1281","repository_url":"https://api.github.com/repos/firehol/netdata","labels_url":"https://api.github.com/repos/firehol/netdata/issues/1281/labels{/name}","comments_url":"https://api.github.com/repos/firehol/netdata/issues/1281/comments","events_url":"https://api.github.com/repos/firehol/netdata/issues/1281/events","html_url":"https://github.com/firehol/netdata/issues/1281","id":191478037,"number":1281,"title":"node.js module performance concerns","user":{"login":"mutsys","id":8465576,"avatar_url":"https://avatars.githubusercontent.com/u/8465576?v=3","gravatar_id":"","url":"https://api.github.com/users/mutsys","html_url":"https://github.com/mutsys","followers_url":"https://api.github.com/users/mutsys/followers","following_url":"https://api.github.com/users/mutsys/following{/other_user}","gists_url":"https://api.github.com/users/mutsys/gists{/gist_id}","starred_url":"https://api.github.com/users/mutsys/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mutsys/subscriptions","organizations_url":"https://api.github.com/users/mutsys/orgs","repos_url":"https://api.github.com/users/mutsys/repos","events_url":"https://api.github.com/users/mutsys/events{/privacy}","received_events_url":"https://api.github.com/users/mutsys/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-11-24T10:01:53Z","updated_at":"2016-11-26T13:48:17Z","closed_at":null,"body":"First off, congratulations on creating an awesome tool. I was absolutely blown away by both the power and the simplicity of **netdata**. However, with all of the effort and concern that has gone into the performance aspects of netdata, it appears that the netdata module for node may not have received the same attention. There are a number of items I noticed that may negatively impact performance and ease of adoption.\r\n\r\n* The collected data is sent to from the plugin to the server by writing to `console.log`, which is then consumed by **netdata** reading from .  `console.log` is a synchronous, blocking call and should not be used in any production application. If you must ship data via `stdout`, there are a number of high-performance, asynchronous logging modules available, such as [Winston](https://github.com/winstonjs/winston).\r\n\r\n> As an aside, why go the `stdout` route of sending data from the plugin to the server? Certainly it is easy, but it would seem that it comes with a whole host of other issues besides performance. The server has to `exec` and then keep track of all of the external plugin processes. This would seem on the surface to be a limit to scaling external plugins since all data collection must reside on the same host as the **netdata** server. A poorly written plugin could steal resources from the server and make it inaccessible. Why not just send some JSON across the network? Why not extend the REST API to allow for `POST` to send data?\r\n\r\n* Timing/scheduling of data collection and generation of timestamps uses `new Date().getTime()` to get milliseconds since the epoch. This is unnecessarily creating `Date` objects at a fairly high frequency. The `Date` object is a good deal larger than a number in javascript (112 vs 16 bytes in V8, according to Chrome profiler). In addition, the amount of code executed when calling `new Date().getTime()` is significantly greater than when calling `Date.now()`. Take a look at [builtins-date.cc](https://github.com/nodejs/node/blob/73ae2d1895c2c0d1d4eeaddd284c58d776d2be87/deps/v8/src/builtins/builtins-date.cc) for the details. `new Date().getTime()` creates a date starting with millis since the epoch, converts it to a date in the local timezone and then converts that back to millis since the epoch again.\r\n\r\n* `logdate` formats a `Date` in a rather brute force manner. There are a number of `toXXXString()` methods on `Date` that will get you the same thing. The `zeropad2` method is not necessary. The same can be done more easily as:\r\n\r\n```javascript\r\nzeropad(s) {\r\n    return (\"00\" + s).slice(-2);\r\n}\r\n```\r\n\r\n* The `stringify` function is being used exclusively to serialize objects to JSON for debugging purposes. It is using `util.inspect`, which can apply a color scheme to the serialized/json output. However, by default, it does not perform this coloring and the option to specify coloring is not being set. Since this feature is not being used, I would suggest using `JSON.stringify()` instead [JSON.stringify() docs](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify). It doesn't provide features you aren't using but it does provide you with the ability to customize the format of its output, applying indentation or custom property conversions if you choose, allowing you to present much more readable output. `util.inspect` just writes its output as flat lines, a rather unpleasant presentation if you are trying to debug your plugin.\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/firehol/netdata/issues/comments/263064366","html_url":"https://github.com/firehol/netdata/issues/1281#issuecomment-263064366","issue_url":"https://api.github.com/repos/firehol/netdata/issues/1281","id":263064366,"user":{"login":"ktsaou","id":2662304,"avatar_url":"https://avatars.githubusercontent.com/u/2662304?v=3","gravatar_id":"","url":"https://api.github.com/users/ktsaou","html_url":"https://github.com/ktsaou","followers_url":"https://api.github.com/users/ktsaou/followers","following_url":"https://api.github.com/users/ktsaou/following{/other_user}","gists_url":"https://api.github.com/users/ktsaou/gists{/gist_id}","starred_url":"https://api.github.com/users/ktsaou/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ktsaou/subscriptions","organizations_url":"https://api.github.com/users/ktsaou/orgs","repos_url":"https://api.github.com/users/ktsaou/repos","events_url":"https://api.github.com/users/ktsaou/events{/privacy}","received_events_url":"https://api.github.com/users/ktsaou/received_events","type":"User","site_admin":false},"created_at":"2016-11-26T13:48:17Z","updated_at":"2016-11-26T13:48:17Z","body":"So, based on your suggestions:\r\n\r\n1. console.log usage for sending data to netdata.\r\n\r\n  Winston has a lot of dependencies and I am not sure it is really needed. I'll investigate alternatives. If you take a closer look, you will see that each module is calling `console.log` just once per iteration (they queue all data and at the `end()` they call `console.log`. The reason I made it that way is to prevent multiplexing the responses of multiple modules/job at netdata (it would be a mess).\r\n\r\n2. I changed all `new Date().getTime()` calls to `Date.now()`. Thanks for this suggestion. I was not aware.\r\n\r\n3. I changed `zeropad2()` as you suggest. `logdate()` is rarely called. It is only called to log error messages and debugging info. I needed a way to have a uniform date logged at `netdata.log` and I couldn't find a better way to have node.js log in exactly the same format all the other plugins and netdata do. So, I compose it.\r\n\r\n4. You are right about the `stringify()` call. If you check the commit log, you will see that there was a version using `JSON.stringify()`. However, `JSON.stringify()` does not allow setting a depth and in certain cases this resulted is really huge output. So, I used `util.inspect` to solve that problem. This call is again only for error logging and debugging, so it is rarely used.\r\n\r\nOverall: the change of `new Date().getTime()` really made a difference. This is called everywhere (web dashboard and node.js plugins) all the time. I have not measured the final effect on netdata by replacing it with `Date.now()`, but I can tell the netdata dashboard feels snappier.\r\n\r\nVery nice catch! Thank you for taking the time to suggest this change!\r\n\r\n"}}
{"actor":{"display_login":"ktsaou"}}