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

Red Hat NFVPE Sysevent Plugin #2624

Open
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@abays
Copy link
Contributor

commented Dec 13, 2017

ChangeLog: Sysevent plugin: A new plugin that monitors rsyslog for system events.

abays added some commits Dec 13, 2017

Merge pull request #3 from atyronesmith/sysevent
sysevent plugin initial commit
@abays

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

We are going to convert this plugin to use notifications (events) as opposed to values (metrics). Please stand by.

abays added some commits Feb 26, 2018

@leifmadsen

This comment has been minimized.

Copy link

commented Apr 17, 2018

Bump for review?

@leifmadsen

This comment has been minimized.

Copy link

commented Apr 27, 2018

You'll also want to adjust the spec file so that the module is added to the RPM when it is built:

https://github.com/collectd/collectd/blob/master/contrib/redhat/collectd.spec

@paramite

This comment has been minimized.

Copy link

commented Jul 2, 2018

+1

@mrunge

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

looks good to me

@@ -6623,6 +6625,7 @@ AC_PLUGIN([snmp_agent], [$with_libnetsnmpagent], [SNMP agent plugin])
AC_PLUGIN([statsd], [yes], [StatsD plugin])
AC_PLUGIN([swap], [$plugin_swap], [Swap usage statistics])
AC_PLUGIN([synproxy], [$plugin_synproxy], [Synproxy stats plugin])
AC_PLUGIN([sysevent], [$with_libyajl], [Syslog event statistics])

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 13, 2018

Contributor

[Events from rsyslog] is more accurate

@anaudx

anaudx approved these changes Jul 16, 2018


// *** END syslog fields ***

if (yajl_gen_map_close(g) != yajl_gen_status_ok)

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 16, 2018

Contributor

duplicate line 390

This comment has been minimized.

Copy link
@abays

abays Jul 18, 2018

Author Contributor

This isn't actually a duplicate. The yajl_gen_map_close on line 390 adds the closing bracket for the "syslogs fields" sub-map, while the yajl_gen_map_close on line 395 closes the encompassing map.

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 19, 2018

Contributor

oh my mistake

if (yajl_gen_get_buf(g, &buf2, &len) != yajl_gen_status_ok)
goto err;

*buf = malloc(strlen((char *)buf2) + 1);

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 16, 2018

Contributor

return value of malloc should be checked if NULL

This comment has been minimized.

Copy link
@abays

abays Jul 19, 2018

Author Contributor

Thanks for spotting this.

ring.head = 0;
ring.tail = 0;
ring.maxLen = buffer_length;
ring.buffer = (char **)malloc(buffer_length * sizeof(char *));

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 16, 2018

Contributor

return value of malloc should be checked if NULL

This comment has been minimized.

Copy link
@abays

abays Jul 19, 2018

Author Contributor

Thanks again for spotting this one too.

ring.buffer = (char **)malloc(buffer_length * sizeof(char *));

for (int i = 0; i < buffer_length; i++) {
ring.buffer[i] = malloc(listen_buffer_size);

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 16, 2018

Contributor

return value of malloc should be checked if NULL

sysevent_thread_loop = 0;
ERROR("sysevent plugin: starting thread failed.");
pthread_mutex_unlock(&sysevent_lock);
return (-1);

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 16, 2018

Contributor

parenthesis are not needed

}

pthread_mutex_unlock(&sysevent_lock);
return (0);

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 16, 2018

Contributor

parenthesis not needed here and in all such statement below

ci->values[1].type != OCONFIG_TYPE_STRING) {
ERROR("sysevent plugin: The `%s' config option needs "
"two string arguments (ip and port).",
ci->key);

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 16, 2018

Contributor

can be appended to previous line

This comment has been minimized.

Copy link
@abays

abays Jul 18, 2018

Author Contributor

This break-up was done by the required clang formatting and needs to stay as-is to avoid violating the styling.

This comment has been minimized.

Copy link
@anaudx

anaudx Jul 19, 2018

Contributor

ok

abays added some commits Jul 18, 2018

@paramite

This comment has been minimized.

Copy link

commented Jul 24, 2018

+1

@rpv-tomsk

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

Hi!

Can you please add a practical example/use-case of this plugin? How it can be used / how do you use it?

Thanks!

@rpv-tomsk rpv-tomsk added this to the 5.9 milestone Oct 14, 2018

abays added some commits Oct 22, 2018

@paramite

This comment has been minimized.

Copy link

commented Apr 15, 2019

@rpv-tomsk @octo Sorry guys for bothering, but is there anything else missing for this patch to get it merged?

@abays

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Wondering about the ChangeLog requirement. Do we need to do something manually to add that? Would just like to help move this forward.

@mrunge

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Andrew,

the ChangeLog feature asks for adding a description to the commit
message, see here:

https://github.com/collectd/collectd/blob/master/docs/CONTRIBUTING.md#changelog

Another example is here: #3139

@abays

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Any chance we can get a review and/or approval/merge for this plugin?

@mrunge

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@rpv-tomsk Pavel, did you had the chance to get back here? Do you see anything missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.