ergochat / ergo

A modern IRC server (daemon/ircd) written in Go.
https://ergo.chat/
MIT License
2.29k stars 181 forks source link

Machine-readable snotices #1546

Open DanielOaks opened 3 years ago

DanielOaks commented 3 years ago

Server notices, etc. These things that opers use to keep an eye on the server.

Would it make sense for us to have a cap that sends those as machine-readable messages instead? e.g.

(without cap)
NOTICE * :Client connected [dan] [u:d] [h:127.0.0.1] [ip:127.0.0.1] [r:This is My Realname, Suckers!!]

(with cap)
SINFO client_connected nick:dan user:d host:127.0.0.1 ip:127.0.0.1 :gecos:This is My Realname, Suckers!!
or
SINFO client_connected :{"nick":"dan","user":"d","host":"127.0.0.1","ip":"127.0.0.1","gecos":"This is My Realname!!"}
or
@oragono.io/sinfo-field-nick=dan;oragono.io/sinfo-field-user=d;oragono.io/sinfo-field-host=127.0.0.1;oragono.io/sinfo-field-ip=127.0.0.1;oragono.io/sinfo-field-gecos=This\sis\sMy\sRealname!! SINFO client_connected
or
SINFO client_connected nick=dan;user=d;host=127.0.0.1;ip=127.0.0.1;gecos=This\sis\sMy\sRealname!!

or um... something? idk, would anyone find something like this useful? mostly useful for bots I think, that try to keep an eye on snotices and parse information out of them to provide more functionality to opers. seeking opinions.

slingamn commented 3 years ago

This seems extremely useful to me, but I think in order to arrive at a useful specification we'll need to collaborate closely with client/bot developers.

JSON seems like a good idea but it may require line continuations in some case, and I don't know how bot developers will feel about those.

DanielOaks commented 3 years ago

Anything will require line continuations in some cases, I think the overhead of json is pretty minimal but we could try to use tag-value escaping if we really felt like it? We could keep it simple, do the CAP solution and have an optional * param that means 'line is not complete', and just make sure we send one SINFO at a time to users.

slingamn commented 3 years ago

Yeah, I think you're right, JSON seems fine.

prdes commented 3 years ago

All of these become easier:

1) oragono-dnsbl snotes eg. if something was flagged yet allowed, why..
2) #1545 3) #1518

and some others which i can't think of right away

progval commented 3 years ago

Why JSON? it's just key-value, so message tags would be a perfect fit

kylef commented 3 years ago

Why JSON? it's just key-value, so message tags would be a perfect fit

👍 we already have multiple ways to send key-value data over IRC (ISUPPORT, capabilities, message tags). It would be nice to not invent another way to do this.

DanielOaks commented 3 years ago

Eh sure why not. My initial thought is that message-tag encoding is prolly a bit tougher for bot devs to parse out but hopefully whatever lib they use should give them easy access to it. I'm not sure whether we should encode them as actual tags on the message or use a param to deliver the string though, if it's actually as tags then we're gonna need to vendor-prefix every single tag name which is annoying, but probably easier for bot devs since they won't need to do anything special at all, and there hopefully shouldn't be any length issues since tag space is already 8k or whatever.

DanielOaks commented 3 years ago
  1. Just encode each value as a separate tag (oragono.io/sno-field-<fieldname>) - Vendor prefixing is loooong but probably easiest for devs to access so long as their lib parses tags properly.
  2. Encoding it as either message-tag-encoding or json as a param - I kinda like json but this idea seems pretty silly, especially if we do define some decent continuation rules for extending sno fields that are particularly long (idk what on earth we're gonna want to throw into sno fields in future, etc).

Hmm, I dunno which one I prefer. I do like the message-tag-encoding-in-param looking thing up in the OP - so long as that can be easily parsed by bots that'd be nice.

DanielOaks commented 3 years ago

Eh, rough thoughts for right now:

SINFO <name> [*] <values>
<name> is a name that we define that identifies what this piece of server info is about, and what
keys should exist in the values. may be versioned with  /num (e.g. client_connected/2 ).
if * is included, then these values should be cached and the values in the next SINFO line should
be appended (continuation lines, same as they work in CAP basically).
<values> is encoded with message-tag encoding.
More parameters may be added on to the end later on, so yeah keep that in mind.

e.g.
SINFO client_connected nick=dan;user=d;host=127.0.0.1;ip=127.0.0.1;gecos=This\sis\sMy\sRealname!!
--
SINFO client_connected * gecos=This\sis\sMy\sRealname\sand\sit\sis\sreally\sreally\slong\sfor\sno\srea
SINFO client_connected son\sat\sall!!;nick=dan;user=d;host=127.0.0.1;ip=127.0.0.1

And I imagine we might just keep a 'registry' of SINFO names, meanings, and keys that go with it. You can additively add keys without a version bump, but if you're taking keys away you need to change the name.


as for why go with the cap-style * continuation rather than using batches or whatever, I think this is the simplest possible solution. we could go all overengineery on this but we want bot authors to be able to use this with zero problems, zero issues, and without any possibility of screwing it up. this feels simple and easy to parse, with little to no overhead.

I mean you have to make sure you're only sending one sinfo packet to each client at a time but I feel like that's a pretty small price to pay given how many clients are likely going to use snotices at all and how many of those are going to use SINFO to receive them.

slingamn commented 3 years ago

Just for the record, I don't understand the objections to JSON; it's definitely easier for me to get my hands on a JSON parser than on something that will parse message-tags in isolation. (It's also conceivable that we might want one of these notices to have richer structure than a flat dictionary.)

slingamn commented 3 years ago

I mean you have to make sure you're only sending one sinfo packet to each client at a time but I feel like that's a pretty small price to pay given how many clients are likely going to use snotices at all and how many of those are going to use SINFO to receive them.

This is a bit of a problem in our implementation, actually, since sno are generated and sent completely asynchronously (like incoming PRIVMSG or whatever). We could make it work if necessary (we might add a special mutex on the client?)

I think this would be fairly simple: the 1-argument form of SINFO is a complete, terminated notice, and the 3-argument form is <id> <i/n> :<notice_fragment>, where <id> indicates a unique ID (opaque bytestring like msgid), where i/n indicates "message i out of n".

progval commented 3 years ago

Just for the record, I don't understand the objections to JSON; it's definitely easier for me to get my hands on a JSON parser than on something that will parse message-tags in isolation. (It's also conceivable that we might want one of these notices to have richer structure than a flat dictionary.)

Because of the complexity; and allowing nested structures is an anti-feature IMO, because it would make it too easy for IRC to become another bloated chat protocol.

kylef commented 3 years ago

Just for the record, I don't understand the objections to JSON; it's definitely easier for me to get my hands on a JSON parser than on something that will parse message-tags in isolation.

My objection isn't about JSON, it's about adding another way to represent key-value's in an IRC message. Effectively, a client would then need multiple different key-value parsers for different messags and different parts of message lines, for (as far as I can tell) little additional value as these examples are using only string values.

slingamn commented 3 years ago

I've come around on this. I think I was stuck in the mindset of thinking of this as something that runs on top of NOTICE, rather than as an extension to the protocol itself. If it's part of the protocol it makes sense to reuse primitives that already exist in the protocol.

But: what if we just put the data itself into the tags section?

progval commented 3 years ago

@slingamn It would need to be namespaced as @DanielOaks pointed out in https://github.com/oragono/oragono/issues/1546#issuecomment-790592223

This sounds good for me.

Plus, if message-tags is negotiated, this doesn't even require the client to request an extra capability. (Which could be an issue with some bots/libs, eg. Limnoria doesn't provide an "official" way for plugins to request caps; but mostly because I've never seen a need for it)

DanielOaks commented 3 years ago

The main reason I'd rather not throw it in the tags section is "oh gods that is the longest message I've ever seen" especially with the namespacing we need to do. If we do that it'd be worth submitting this to ircv3 not to actually get it standardised but just so we can use draft/ instead of oragono.io/ on all the tag names :P

On the upside, we probably don't need continuation because of how long the tag section is?

slingamn commented 3 years ago

Yeah, the wasted bytes are annoying but on the plus side, (a) IRCv3-capable clients don't have to do any of their own parsing (b) we can probably forget about continuations.

DanielOaks commented 3 years ago

Ehsure

DanielOaks commented 3 years ago
oragono.io/sinfo Extension

When the oragono.io/sinfo capability is requested, clients will receive oper server-notices as
machine-readable SINFO messages instead of receiving them as NOTICE messages.

    @values SINFO <type>

<type> is a name that we define that identifies what this piece of server info is about, and what
keys should exist in the values. May be versioned with  /num (e.g. client_connected/2 ).
Each value is included as a tag on the SINFO message, prefixed with oragono.io/sinfo-tag-
More parameters may be added on to the end of the message later on, so yeah keep that in mind.

e.g.
@oragono.io/sinfo-tag-nick=dan;oragono.io/sinfo-tag-user=d;oragono.io/sinfo-tag-host=127.0.0.1;
oragono.io/sinfo-tag-ip=127.0.0.1;oragono.io/sinfo-tag-gecos=This\sis\sMy\sRealname!! SINFO client_connected

**Versioning Considerations**
The (/num) versioning on SINFO types lets us either stop delivering old required tags, or start
delivering new required tags. In most cases, if we screw up an SINFO type definition, we can just
start sending new (optional) tags to fix it, but if we do need to fix it in a more drastic way,
we can use this to start sending a new 'type', with a new definition in the SINFO registery (the
table where we have a list of SINFO types -> tags and all). This is why the versioning is part of
the type name, rather than as part of the SINFO cap/value itself or as part of the tags themselves.

For reference re the 'versioning' of SINFO message types: it's pretty much for if we screw up and introduce an SINFO message with bunk/nonsense tags and we need a way to deliver a new version where we can avoid sending all those tags and deliver new ones instead.

In most cases we can just tack new (optional) tags on that'll fix it, but if we either don't want to send the old (required) tags anymore or want the new ones to be required, new version of that message type (which includes a new entry in the 'sinfo registery' or basically just the table where we list all the messages and details about 'em).

^ is why this version is part of the actual type/name rather than e.g. on the cap or on the tags themselves.

DanielOaks commented 3 years ago

Counter-proposal, all of this info is delivered as tags on top of the existing snotice NOTICEs. How does this feel?

slingamn commented 3 years ago

From discussion, +1 for a new command (particularly since we're already wasting a bunch of bytes by using namespaced tags :-P)

progval commented 3 years ago

A con of adding a new command instead of the NOTICE: it means that if a Limnoria plugin expects a NOTICE but another one requested the cap, the former won't see the notice.

It's a minor edge case, I don't think anyone loads two plugins parsing snotices, but implementing this as a new command would set a precedent

prdes commented 3 years ago

Is it possible to recieve this format of NOTICEs if you have +B on yourself. But Human-readable otherwise. If I'm way off lemme know