udp-notifier mechanism #1445
udp-notifier mechanism #1445
Conversation
| --new-log-timestamp Enable full ISO-8601 timestamp in all logs. | ||
|
|
||
| --new-log-timestamp-format <format> Set timestamp format (in strftime(1) format) | ||
| --new-log-timestamp Enable timestamp in all logs. | ||
|
|
There was a problem hiding this comment.
it seems like you may need to rebase against the latest master/main branch?
Unless your PR is intending to remove the --new-log-timestamp-format option?
| @@ -2328,6 +2329,10 @@ static void set_option(int c, char *value) { | |||
| turn_params.rest_api_separator = *value; | |||
| } | |||
| break; | |||
| case UDP_NOTIFIER_OPT: | |||
| turn_params.udp_notifier = 1; | |||
There was a problem hiding this comment.
please use the C99 "stdbool" header, and the true/false keywords (sometimes implemented as macros by C99) over '1' and '0' and 'int'.
|
|
||
| //// UDP Notifier //// | ||
| int udp_notifier; | ||
| char udp_notifier_params[1025]; |
There was a problem hiding this comment.
1025 is a magic number.
Additionally, does this need to be a flat buffer of characters? can't it be parsed into a struct that has meaningful variable names for whatever the fields are?
|
I suppose that mechanically the change is reasonable for what it's trying to accomplish, but i have some concerns about approach and justification The first observation that I want to make is that this change is hooking only the
Then you're sending network packets with this information in a raw binary format (not a standardized or documented format, that i can tell) to some arbitrary UDP port on some arbitrary ipaddress. Ignoring the question of "Why would you want that?", i have two direct objections to this.
I have a broader, higher level, feedback consideration here as well. Today this is only trying to provide notification support for a single message type. But in general this looks very much like a generic logging mechanism that just happens to only be used for a single log type, why limit to only one, and why roll your own? Surely it would be more straight forward to replace the existing log macros with whatever logging implementation you want (potentially including this notification framework you've built)? If you don't like the current logging interface, this change replaces it with a third party open source logging library, that (I haven't double checked) probably has hooks to handle custom log sinks: #1351 Lastly, the dreaded "Why not X" question. Why not use a well established telemetry system? That's basically what you're looking for right? A way to receive notifications on a separate process / system that can audit commands sent into I recommend instead of implementing an in-house solution for this, use an industry standard. One that my company has been putting a lot of work into adopting recently is OpenTelemetry: https://github.com/open-telemetry/opentelemetry-cpp It provides lots of capabilities for auditing, and the data is vendor / backend agnostic, with the ability to target quite a few existing vendors, or to create your own receiver. |
Hi All,
It allows retrieving information about failed turn command request to generate some statistics.
This PR provides a general notification interface for the Turn server. It generates informational messages and logs for the Turn server.
The UDP interface is implemented.
The notification interface is currently used for "executed turn command".
This pr is written to send information about all client operations performed on TURN Server to Turn Monitor over UDP packets.
The message format is "[Turn Server Command Execution Notification: {"session": "%018llu", "origin": "%s","realm": "%s","user": "%s", "connection": "%s", "method": "%d", "response": "%d", "reason": "%s"}]" is.