Add UDP Request/Response for JSON formatted server status #2006
Conversation
|
Thanks for this - can you give it a more descriptive name (something like "JSON for recording status"?) and some more info on the use case for it? |
I'm pretty new to using github with forked projects. Please bear with me as I navigate the learning curve. I've updated the commit name. |
|
Perfect, thanks! |
|
Thanks for the work and the PR, @Rob-NY! While I appreciate that it solves a particular use case for you, I'm wondering if it would be better incorporated into the other JSON work being done in #1975, which uses TCP. Having two separate, different types of JSON interface (one UDP and one TCP) seems less than elegant. Disclaimer - I haven't yet had the time to study #1975. |
|
@softins - Yes, I understand that concern and agree that there may need to be a choice made here. While #1975 beat me to the PR, I surfaced my suggestion in June. Further, my approach does not introduce a new messaging protocol (TCP), and it behaves in exactly the same way as all other Jamulus messages. Further, it only "fills the gaps" for things that are not available today - specifically retrieving attributes of the connection information and the current recording state of the server. |
|
Hi, thanks for opening this PR. Definitely the code looks smaller and probably easier to maintain. I think it’s speed vs features vs maintenance vs potential bugs. |
|
@Rob-NY the styling Check failed. Could you please run clang-format? |
| " -w, --welcomemessage welcome message to display on connect\n" | ||
| " (string or filename)\n" | ||
| " -z, --startminimized start minimizied\n" | ||
| " --serverpublicip specify your public IP address when\n" |
There was a problem hiding this comment.
You've managed to duplicate the help text some how.
| " running a slave and your own directory server\n" | ||
| " behind the same NAT\n" | ||
| " --serverbindip specify the IP address the server will bind to\n" | ||
| " --notifyserver specify server to receive UDP notifications IP:PORT\n" |
There was a problem hiding this comment.
I'd suggest grabbing the short option -N and inserting this in the right order in the help text (alphabetical by short option, then any closely related options without short options).
| if ( !strNotifyServerAddr.isEmpty() ) | ||
| { | ||
|
|
||
| // No IPv6 here |
|
|
||
| if ( NetworkUtil::ParseNetworkAddress ( strNotifyServerAddr, addrNotifyServer, false ) ) | ||
| { | ||
| qInfo() << "-Server notifications sent to:" << addrNotifyServer.toString(); |
| * write to a disk-based status file, or sent via CL message. | ||
| */ | ||
|
|
||
| void CServer::BuildServerStatusJson ( QString& strServerStatus ) |
There was a problem hiding this comment.
What's the maximum size of this JSON object?
|
Converting to draft for now. |
|
@Rob-NY if you want, you can add yourself as contributor here: Line 493 in 89b3d0e |
|
That is very kind of you. Thank you.
… On Feb 16, 2022, at 4:34 PM, ann0see ***@***.***> wrote:
@Rob-NY if you want, you can add yourself as contributor here: https://github.com/jamulussoftware/jamulus/blob/89b3d0e9604aac7a8b2d241e351e82247b93457e/src/util.cpp#L493 and - if you want - you can add your project to the RELATED-PROJECTS.md file. We really appreciate your contribution, but it seems as if the JSON-RPC approach has more support. However - since you contributed valuable ideas, we would like to honor your work.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you were mentioned.
|
|
@Rob-NY would you be ok in closing this in favor of JSON-RPC? If anything from this is needed in future, I think it would be good to have it on your repo (i.e. don't delete it). Nevertheless, thank you very much for your efforts and contributions for both API-approches ;-). |
|
Happy to. I’m still maintaining this for my own use and need to make it available under the license, but it probably doesn’t need to sit as a PR. I’m not a GitHub expert so let me know how to “close” or “cancel” this or if you can do it, go ahead.
… On Mar 4, 2022, at 3:26 PM, ann0see ***@***.***> wrote:
@Rob-NY would you be ok in closing this in favor of JSON-RPC? If anything from this is needed in future, I think it would be good to have it on your repo (i.e. don't delete it).
Nevertheless, thank you very much for your efforts and contributions for both API-approches ;-).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you were mentioned.
|
|
I'll close it for you. Thanks anyways (and it's still available on your repo so not lost). |
Implement two UDP messages (request and response) to send JSON based server status message containing information not readily available through other means. The JSON status includes connected client information including IP address as well as the current "real" recording state of the Jamulus server. Further, a command line option is specified which both enables/disable this functionality as well as limits the IP addresses to which this JSON data can be sent.
The use cases to support this functionality rest in a server administrators ability to 1) Know the IP address of connected clients for the purpose of blocking (and subsequently re-admitting) clients; 2) To query the current recording state of the server. Both of these will enable remote server control and monitoring in a Jamulus-ness way using existing protocols and message structures.
Live system using a version of Jamulus server with this PR is available at https://vjammers.com (See #1847 and #1990)
JSON formatted server status message that includes the IP address of connected clients as well as the true, current recording state of the Jamulus server. A command line option enables/disables this functionality and restricts the IP address that is permitted to request this status.
This change will require documentation. I'm willing to write that documentation if PR is approved.
Status of this Pull Request
What is missing until this pull request can be merged?
Only tested on LINUX. Do not have the ability to test on Mac or Windows.
Checklist