-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSC2063: Add "server information" public API proposal #2063
base: old_master
Are you sure you want to change the base?
Conversation
Mentioned in the text is #1929 . |
Signed-off-by: Peter Gervai [email protected] |
I support this change because we need it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea; but to become a real proposal it needs elaboration. At the very least, please address comments below.
Mention its existence but remove from the main body, these all shall be covered by pull#1929. Also remove specifics from *server connectivity checking* which shall be covered by a separate proposal (as federation pin, traceroute, or else).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking on one remaining thing, but otherwise this looks pretty good to me.
"server_data": { | ||
"m.open_registrations": true, | ||
"m.uptime": 63072000, | ||
"m.registered_users": 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost perfect; but it'd be probably better to further distinguish between statistics/status information (uptime and the number of users) and configuration/settings information (open registrations). I don't know if it's worth it to introduce a separate /server_config
endpoint just for a single boolean value - probably overengineering here. It would be great to have other opinions on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
{
"server_data": {
"settings": {
"m.open_registrations": true
},
"statistics" : {
"m.uptime": 63072000,
"m.registered_users": 4
}
}
}
Processing a json and ignoring unnecessary fields is probably painless enough that it doesn't justify creating any more complexity.
I guess someone shall come up with a real justification to separate them even this way though, or maybe it's good for "visibility" or "mental separation"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather use two separate endpoints. The reason is that open registrations may (I can imagine that in theory at least) be configurable using some administration API. What I mean to say is that generally configuration is, at least in theory, read/write depending on your access credentials, while statistics are produced by the servers and cannot be tweaked even in theory. But probably let's validate it in the Matrix room.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be pointed out that open_registrations is not something federation should care about. It should be reported by a client API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I think it's fine to expose health/metadata/metrics at the federation level for this, mostly because there's a discovery function that server lists can use to query the data.
Syncing here comment on MSC1929 regarding this, that MSC and ServerInfo. |
{ | ||
"server_data": { | ||
"m.open_registrations": true, | ||
"m.uptime": 63072000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering what usefulness the uptime actually has? If it is small, it could be "they updated recently == good" or "they're in a crash loop doing constant OOM == bad". If it is big it could be either "it's stable" or "it's not being maintained". I don't know what would be better though, just wondering whether it is good to include numbers that don't really have any particular purpose. This feels more something that should live in the /metrics
endpoint for prometheus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include numbers that don't really have any particular purpose.
If you do not need it doesn't mean nobody else does. I could describe how it's useful but really, I believe that "it is useful for some people" should be enough to justify.
This feels more something that should live in the /metrics endpoint for prometheus.
This is a public endpoint, in contrast to prometheus. The primary point is to provide data for automated public server lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh the first thing I'd disable is uptime: there's no metric that can be pulled from it which helps users with anything. How does uptime apply to a worker or multi-process homeserver for instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does uptime apply to a worker or multi-process homeserver for instance?
From the users viewpoint a service is "UP" when it's working. :-)
(As a user I would not pick a server with a constant less than a few days uptime, since it's either keep restarting or crashing, but that's me.)
But maybe you're right that it should be externally measured [icmp/api calls] instead of relying on the server.
The fediverse Aggregator site for example expects to display the following stats for nodes:
|
@grinapo you'll need to hard reset to before the merge - a revert isn't going to work. If you need help, visit #matrix-spec:matrix.org and we can figure it out there. |
rendered