sipwise / rtpengine

The Sipwise media proxy for Kamailio
GNU General Public License v3.0
784 stars 368 forks source link

Adding support for NG trace to Homer #1802

Closed lbalaceanu closed 5 months ago

lbalaceanu commented 7 months ago

Proposing a patch to also send NG traffic to sipcapture servers.

For a sipcapture server one can use Kamailio and insert from the config similar to https://github.com/sipcapture/homer-config/blob/master/sipcapture/sipcapture.kamailio5.

I am setting the protocol type to 0x3d (61) - rtcp homer fills it with 0x05.

Support is desired for Kamailio/Rtpengine traffic via UDP. Adding homer-enable-rtcp-stats and homer-enable-ng config params to separately control sending to Homer each traffic type. By default rtcp is on when homer parameter is configured. NG is by default off.

Any comments are appreciated, Thank you

lbalaceanu commented 7 months ago

Hi @rfuchs, do you consider such an addition welcomed? Thank you.

rfuchs commented 7 months ago

Hi @rfuchs, do you consider such an addition welcomed? Thank you.

Yes absolutely. I'll take a closer look at this when I find some spare time.

I assume the protocol value of 0x3d has been registered on the Homer side, or is a free-form value?

lbalaceanu commented 7 months ago

Hi Richard,

I just picked this 0x3d value as first one free in the HEP3 protocol specification, maybe making the variable configurable is the way to go.

From what I know, there is no official sipcapture/Homer UI support for capturing/displaying NG values. One can still look the data up in the database itself.

At 1&1 we use an old Homer interface and with little hacks this NG can be displayed alongside the SIP itself just as long as the SIP and the NG messages are stored in the same tables.

Thank you

lbalaceanu commented 6 months ago

I tried addressing all but the first comments and I now get an error when the unit tests are run. I am a bit at a loss to identify which scenario detects the problem.

Thank you

rfuchs commented 6 months ago

That's libasan complaining. There's a mem leak.

==340490==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 18120 byte(s) in 755 object(s) allocated from:
    #0 0x7f97594f3bd7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x56233954ab4a in cache_entry_dup ../include/cookie_cache.h:25
    #2 0x56233954ab4a in cookie_cache_insert /home/dfx/src/ngcp-git/rtpengine/daemon/cookie_cache.c:74
    #3 0x56233956d132 in control_ng_process /home/dfx/src/ngcp-git/rtpengine/daemon/control_ng.c:509
    #4 0x56233956ea7a in control_ng_incoming /home/dfx/src/ngcp-git/rtpengine/daemon/control_ng.c:554
    #5 0x56233954e28a in udp_listener_incoming /home/dfx/src/ngcp-git/rtpengine/daemon/udp_listener.c:56
    #6 0x562339b003bc in poller_poll /home/dfx/src/ngcp-git/rtpengine/daemon/poller.c:224
    #7 0x562339b02e72 in poller_loop /home/dfx/src/ngcp-git/rtpengine/daemon/poller.c:321
    #8 0x56233940ec82 in thread_detach_func /home/dfx/src/ngcp-git/rtpengine/daemon/helpers.c:246
    #9 0x7f975945ae65 in asan_thread_start ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234

SUMMARY: AddressSanitizer: 18120 byte(s) leaked in 755 allocation(s).

You can run this locally with make asan-check. Don't forget to clean sources between builds.

lbalaceanu commented 6 months ago

Hi,

I also pushed a commit solving the observation that some code could be moved after a switch. Will test a bit more tomorrow, but more or less this would be my proposal for the code change.

Thank you

rfuchs commented 6 months ago

LGTM now, thanks

lbalaceanu commented 6 months ago

Hi, I also have no more additions to the code. Thanks.

rfuchs commented 6 months ago

Actually one last request, to please document the new options in the man page :laughing:

lbalaceanu commented 6 months ago

Hi, sure, just updated the docs.

lbalaceanu commented 5 months ago

Hi @rfuchs,

In case there is something more to do about this PR, please tell me.

Thank you

rfuchs commented 5 months ago

Hi @rfuchs,

In case there is something more to do about this PR, please tell me.

Thank you

Just waiting for internal review but I think I'll just push it through...