Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added escape of items with " #31

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

rtshome
Copy link

@rtshome rtshome commented Oct 6, 2018

The pm2 processes that have a name with square brackets must be escaped.
I added a generic escape for all keys using double quotes as zabbix documentation states

Copy link
Member

@rkaw92 rkaw92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A necessary change overall, but needs some minor tweaks:

  • Avoid changing all tabs to spaces in the source please, leave them as they are - any editor/IDE should support this; otherwise we generate unnecessary line changes.
  • See note about matching/escaping.

@@ -72,7 +72,11 @@ PM2ZabbixMonitor.prototype._initListeners = function _initListeners() {
* @returns {string} The item key to send the data to Zabbix with.
*/
PM2ZabbixMonitor.prototype.getDataKey = function getDataKey(processID, processState, dataItem) {
return 'pm2.processes["' + processID + '",' + dataItem + ']';
if (processID.match(/[\[\],]/)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • According to the zabbix_sender manpage, whitespace also needs quoting.
  • It may be cleaner to put the string escaping logic in a separate function, so that getDataKey only knows about the prefix and delegates the escaping of strings to somewhere else (after all, it may turn out that dataItem must be escaped in the same way).

@rtshome
Copy link
Author

rtshome commented Oct 16, 2018

@rkaw92 give a look at the latest commit and let me know if it's ok for you.

@dcdamien
Copy link
Member

Hi @rtshome, do you still want to fix it? Please take a look on the last @rkaw92 review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants