Gelf event handler

[mirkobrankovic]

Simple TCP/UDP socket event handler to gelf interface.
Config file is simple, you need filter, host, port

will look like this in Graylog interface

Still need to think about message structure and custom fields.

[lminiero]

Cool, thanks! You may want to rename this PR to event handler, rather than forwarder, though, as I thought this was media-related.

Being UDP based, how does this handle big events? SDP offer/answers can go way beyond the MTU, and it will be even worse once we merge the unified-plan branch. I'm not familiar with GELF, so I'll look into it and review the code.

[mirkobrankovic]

Indeed, there is udp chunking with magic number and packet sequence:
https://docs.graylog.org/en/3.1/pages/gelf.html#gelf-via-udp
Need to look at that also.

Looks like I can't change the title, but maybe need to start new PR?
Anyway i will test it in Acceptance and see how it will behave.

Most importantly is that if event handler can't connect to gelf that core and rest of the plugins work normally.

Update: title changed in desktop version

[lminiero]

Yep, the chunking part looks useful, and so does compression (but that would be true for other plugins as well, I'll have to check that). Do you plan to add chunking to the PR?

[mirkobrankovic]

I will give it a try :D

[mirkobrankovic]

@lminiero can you help me a bit because I got stuck at this chunking and can't resolve it.
Here is the test I did:
I have a simple server.c listener and client.c that is suppose to take 2404 length SDP and chunk it into 512 bytes packets and send it to server.

You can see the result in this result
So I calculate that I need 4*512 and last 356 length.

1. Problem is that my "empty" chunk is not empty and reports 518 length at first.
2. Problem is that first iteration makes a good chunk, except it has additional 6 bytes (for example:L �FE �), but then in second, buf pointer is moved correctly to position 512, but chunk stays the same after strncpy, only those 6 last bytes are removed. Third iteration for chunk var is actually what was suppose to happen in second one.

I'm doing something wrong with this pointer or this 6 bytes are messing everything up, but i'm not sure where they come from.

Hope you have time to help,
Thanks,
Mirko

[lminiero]

Probably some strings manipulation error? Sorry, can't look into this right now.

[atoppi]

Hi @mirkobrankovic,
your client loop should be like this

    //--------------NETWORK PART---------------------------

    int total = len / MAX_GELF_MSG_LEN + 1;
    fprintf(stderr, "Total packets: %d\n", total);
    fprintf(stderr, "----------------------------------------------------------------------------------------------------------\n");

    int offset = 0;
    int sent = 0;
    char *rnd = randstring(8);

    //fprintf(stderr, "Buffer: %s\n", buffer);

    for (int i = 0; i < total; i++) {
        fprintf(stderr, "Packet No: %d\n", i+1);
        fprintf(stderr, "offset: %d\n", offset);
        
        int bytesToSend = offset + MAX_GELF_MSG_LEN < len ? MAX_GELF_MSG_LEN : len - offset;
        fprintf(stderr, "Bytes to send: %d\n", bytesToSend);
        
        sent = write(sockfd, buf+offset, bytesToSend);
        if (sent < 0) {
            fprintf(stderr, "ERROR writing to socket\n");
            printf(" Value of errno: %s\n ", strerror(errno));
            return 1;
        }
        
        fprintf(stderr, "Bytes sent: %d\n", sent);
        offset += sent;
        fprintf(stderr, "----------------------------------------------------------------------------------------------------------\n");
    }

[mirkobrankovic]

@atoppi thanks a lot for pointers :)

[mirkobrankovic]

TCP is working, now to fix UDP.
I will keep it running on our servers and logging to Graylog to see if something bad will happen :D

[lminiero]

If this Gelf thing supports gzip, you may want to have a look at the utils I added recently for the purpose, which should help reduce the size of data on UDP (unless Gelf has its own fragmentation mechanisms).

[mirkobrankovic]

@lminiero thanks I saw it the other day and thought about it.
they do have custom UDP chunking imitating what TCP does, but for compression they do recommend Gzip or Zlib and magic byte is automagically picked up

[mirkobrankovic]

TCP compression is not supported cause of the '\0' but for UDP it is possible, just need to see if then I need to stop chunking or not ....

[mirkobrankovic]

wow it is all connected :D

[mirkobrankovic]

Tested all cases and there is the network sniff:
https://pastebin.com/47KqcBus
@lminiero I would like your code x-ray input, I know you have at least 10 improvements :D

[mirkobrankovic]

I reviewed all 3 segments of the comments and I think I addressed most of them.

This one if puzzling me, what did you mean by this, to decide if message should be sent uncompressed or not?:

				JANUS_LOG(LOG_ERR, "Failed to compress event (%zu bytes)...\n", strlen(message));
				/* Sending message uncompressed */

lminiero on Feb 24 Member
This hasn't been addressed, and I think it's an important point.

Thanks a lot for your time to do this, I really appreciate it.

PS. unfortunately Github is not giving the best user experience with this comment/review/commits all in one thread view

[lminiero]

Thanks! I added some more notes.

[mirkobrankovic]

addressed invalid values:

[Fri Apr 24 12:40:31 2020] [janus.eventhandler.gelfevh.jcfg]
[Fri Apr 24 12:40:31 2020]     general: {
[Fri Apr 24 12:40:31 2020]         enabled: true
[Fri Apr 24 12:40:31 2020]         events: all
[Fri Apr 24 12:40:31 2020]         backend: graylog.coligo.com
[Fri Apr 24 12:40:31 2020]         port: 12201
[Fri Apr 24 12:40:31 2020]         protocol: udpp
[Fri Apr 24 12:40:31 2020]         max_message_len: safdsf512
[Fri Apr 24 12:40:31 2020]         compress: false
[Fri Apr 24 12:40:31 2020]         compression: 6
[Fri Apr 24 12:40:31 2020]     }
[Fri Apr 24 12:40:31 2020] [WARN] Missing or invalid transport, using default:UDP
[Fri Apr 24 12:40:31 2020] [WARN] Missing or invalid max_message_len, using default: 500

@lminiero when you get time, can you review new changes ? thanks

[lminiero]

Thanks! I did another round of review: some are just nits, but there are a couple of things that I think need to be addressed.

[mirkobrankovic]

@lminiero I refactored tcp send to make sure all bytes are sent, but while debugging I haven't seen sent less then size (every time is sent in one go, even when messages are 2500 chars long), but better to be safe.
Also I needed to add NULL terminator, otherwise it was skipping every second message.

Can you do one more round?

[lminiero]

Sorry if this took so long, I must have missed your last message, and I came back here after the discussion on the group about that other effort related to GrayLog. I added a few notes: very minor, as you'll see, so I think we're very close to merging (for real this time 😄 ). I'll check how complex installing a Graylog instance for testing is, to see if I can test it myself, but that would be just out of curiosity: you've been using it long enough, so if it works for you, I'm sure it does its job!

[lminiero]

I think it's better to wrap the items with a math operation in brackets, here, or there may be ambiguities: if not for the compiler, for who reads the code for sure.

[lminiero]

I think this has been around long enough (I see you opened the PR during JanusCon 😱 ), and I think it's fine as it is (I can fix small nits myself, if I find any). Thanks again, merging!

[lminiero]

Made some minor changes (mostly code style), and changed the configure to by default compile the module: your patch had maybe in there, but that's actually only supposed to be used when there are dependencies to take into account, and a check later might turn the maybe into a yes. Since your module doesn't depend on any library, it can be a yes right away (unless something else sets it to no).

[mirkobrankovic]

Thanks a lot, I have a lot of problems with this VS Code style and auto-correct on different languages :D