feat(manager): move and update metrics cards in the server view #2241
Conversation
daniellacosse
commented
Oct 14, 2024
•
daniellacosse
commented
daniellacosse
changed the title
daniellacosse
changed the title
daniellacosse
force-pushed
the
daniellacosse/tunneltime/move_metrics
branch
from
55108a4 to
ee8c388
Compare
There was a problem hiding this comment.
As discussed offline, Per-ASN breakdown was a significant gap during the Iran crisis. It's a confirmed need and and we put a lot of effort to implement it. It's more of a question of how, not whether we want to surface that.
I would like to see that surfaced somehow in this PR, to inform the UX design. We've discussed a simple table, which seems to work well.
Sure thing, working on it! |
Here's an initial PR for a data table component: #2273 |
daniellacosse
changed the title
Co-authored-by: Vinicius Fortuna <[email protected]>
Co-authored-by: Vinicius Fortuna <[email protected]>
|
After testing with the new canary server, this is basically working, except the fact that the data flow in our UI is bad and not triggering a render as it should. (to get it to show up in the UI in the above screenshot I had to edit the code to trigger a live reload LMAO) We should be passing data through HTML attributes not via JS properties... Anyway, I'll return after the holidays to fix that, format the hourly units properly, and add a unit test. |
| (await this.getSupportedExperimentalEndpoints()).has('server/metrics') | ||
| ) { | ||
| return this.api.request<server.ServerMetricsJson>( | ||
| 'experimental/server/metrics?since=30d' |
There was a problem hiding this comment.
I find the discrepancy between the checked URL (server/metrics) and the used URL (experimental/server/metrics) unnecessary. Why not just keep the experimental/ prefix in both to avoid mapping on both sides?
There was a problem hiding this comment.
The only reason why I didn't is because i thought it might be redundant with the name of the method. And that we shouldn't call the method getSupportedEndpoints because I would expect that to return ALL the endpoints. @fortuna thoughts?
| } | ||
|
|
||
| getName(): string { | ||
| return this.serverConfig?.name; | ||
| } | ||
|
|
||
| async getSupportedExperimentalEndpoints(): Promise<Set<string>> { |
There was a problem hiding this comment.
Honestly, I find the use of this separate check and set unnecessarily complicated. Soon the new metrics server URLs will be non-experimental and this code will be dead or need reworking. Will this actually be reused?
There was a problem hiding this comment.
I don't have a strong opinion here. I am attempting to anticipate what @fortuna wants. It became clear that to test this I would need to add a property regarding the experimental endpoints to the root server model and noticed there were only set/get methods there for some reason.
Worth noting there is currently one other experimental endpoint on the server, "access-key-data-limit": https://github.com/Jigsaw-Code/outline-server/blob/13f62390bfda8cd0ac9215aad685ae82f82a1270/src/shadowbox/server/api.yml#L518
There was a problem hiding this comment.
Yeah agreed, though that endpoint is just a redirect. I wonder if we can just remove it from api.yaml instead of marking it as deprecated? I'm not sure what the best practice is on that.
I would kind of prefer an inline "try and fallback if 404" from a readability perspective, but hesitate to recommend it because it adds unnecessary requests for a while for users on old versions.
There was a problem hiding this comment.
I would kind of prefer an inline "try and fallback if 404" from a readability perspective, but hesitate to recommend it because it adds unnecessary requests for a while for users on old versions.
Yeah, that is what I had initially before the point about unnecessary requests was raised.
Co-authored-by: Sander Bruens <[email protected]>
