Skip to content
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

New network centric view #750

Merged
merged 11 commits into from
Jun 1, 2022
Merged

New network centric view #750

merged 11 commits into from
Jun 1, 2022

Conversation

majakomel
Copy link
Contributor

Closes #744

@majakomel majakomel self-assigned this Apr 25, 2022
@vercel
Copy link

vercel bot commented Apr 25, 2022

@majakomel is attempting to deploy a commit to the OONI Team on Vercel.

A member of the Team first needs to authorize it.

@majakomel
Copy link
Contributor Author

@hellais This is a rough implementation of the network view - it needs some more refactoring/cleanup and changes to the date selector when the other ticket is ready to be merged.

Is there any clear idea yet what to put on the top of the page in the "high-level stats" area? I can start working on that part too.

@hellais
Copy link
Member

hellais commented Apr 25, 2022

Thanks for putting this together. I left in the original issue some ideas for what we can do with the high level stats: #744 (comment).

Re: this branch, here is some feedback:

  1. Because of how the API works, asking for AS1234 is the same as asking for 1234. I think on these pages, though, we should enforce stricter checks on what is an allowed ASN (we should ensure it starts with AS). Not doing so will lead to two pages having the same content which, amongst other things, is bad for SEO.
  2. We should have a better page for when a network does not exist. I would say we can detect when we have no data for a particular network and display something like "This network either does not exist or we have no data for it"
  3. The AS number should be displayed prominently at the top of the page
  4. There is something odd going on with the web connectivity chart. The scrollbar is not visible. I recall us discussing this in the past with @sarathms and I think support for setting the scrollbar as visible was added.

pages/network/[asn].js Outdated Show resolved Hide resolved
pages/network/[asn].js Outdated Show resolved Hide resolved
@majakomel
Copy link
Contributor Author

@hellais That for the feedback and review, although this is just a first draft and there are several parts to still be refactored.
I'll add the fixes based on the feedback + more meaningful info to the top of the page and let you know when it's ready for review.

@majakomel majakomel marked this pull request as ready for review May 3, 2022 19:58
@majakomel
Copy link
Contributor Author

@hellais I refactored all charts into one component and extracted two fetchers. Should I add a TODO comment or is there a way to add a follow-up ticket when backend adds support for multiple test_names?

Few notes:

  • I downgraded @nivo/core until the v0.79.1 with regression fixes is published (ResizeObserver not defined for mobile screens plouc/nivo#1928)
  • For now I didn't add custom ranges - the colors are distributed based on how the library works by default (calculated based on min/max of the whole data range). There seems to be the functionality to define a custom range, but it's not documented so I have to yet figure it out.
  • Can you give me some more background on the scrollbar issue on the web connectivity chart? Currently it looks the same as on MAT page, but maybe I'm missing something.

@majakomel majakomel requested a review from hellais May 16, 2022 12:55
utils/index.js Outdated Show resolved Hide resolved
pages/network/[asn].js Outdated Show resolved Hide resolved
@vercel
Copy link

vercel bot commented May 16, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
explorer ✅ Ready (Inspect) Visit Preview May 30, 2022 at 3:01PM (UTC)

@hellais
Copy link
Member

hellais commented May 16, 2022

This is looking good.

Some comments based on an inspection of the preview:

Screenshot 2022-05-16 at 16 26 39

In the measurement overview screen I think we should color code the dates in the future differently, since it's expected for there to not be any data there. I would suggest using an even lighter grey for those dates.

For the dates which have zero data, it would be good if we had in those cases also a tooltip that shows 0 in it, so it's clear that is what is going on here.

For the "Network observed in countries: XXX", I think we should make the XXX country name a link to the relevant country page so that we connect these two pages together.

When I access a network for which we don't have any data, I think we should have a specific 404 page or have a zero state for that network page. We can display something similar to when we have no OONI data for a certain country encouraging people to run measurements: https://explorer.ooni.org/country/AQ.

I think the URLs for the networks should start with AS{NUMBER}, this gives us the flexibility to re-route endpoints based on the prefix if we ever decide to change the format (maybe we want to put the network name in the address bar instead of the ASN).

Sorry if my initial comment on this point wasn't clear.

Answering your questions in the above comment:

Should I add a TODO comment or is there a way to add a follow-up ticket when backend adds support for multiple test_names?

I would say we should document it in an ooni/explorer issue and link it to the backend one so we remember to do it once there is backend support.

For now I didn't add custom ranges - the colors are distributed based on how the library works by default (calculated based on min/max of the whole data range). There seems to be the functionality to define a custom range, but it's not documented so I have to yet figure it out.

I think it would be good if we could figure out how to get the ranges to work properly. The issue with using ranges that are relative to a given network is that it will make it hard to compare charts of different networks (for example what is colored green in one network, might be colored red in another, because they have big spikes of many more measurements).

Can you give me some more background on the scrollbar issue on the web connectivity chart? Currently it looks the same as on MAT page, but maybe I'm missing something.

I tested this again with the latest deployed version and I don't seem to be able to reproduce this anymore.

@hellais
Copy link
Member

hellais commented May 16, 2022

Another thing that would be good to have, is if we could sort the list of "Network observed in countries" by the number of measurements and perhaps also display the total number of measurements for each country.

This is useful because it would allow us to spot cases in which the network is wrongly geolocated to a specific country.

@majakomel
Copy link
Contributor Author

I addressed all the points from the comments, color range is also set now. Empty network page only shows CTA - let me know if you want to add more content.

@hellais
Copy link
Member

hellais commented May 30, 2022

I think this looks good and we should merge and deploy it as is.

We should document some items to be done for future work, which can be implemented iteratively as follow up PRs:

  • Move the loading of the calendar data into the client side. Currently the first load for some very big networks will take even in the range of seconds. We should instead be doing this client-side and display a loader while that is being done.
  • Fetch the calendar data lazily. There is no need to prefetch all the calendar data until the over has opened it up. The only metadata we need to populate that section is the data of the first measurement. We can do this as a dedicated API endpoint and while we are at it we could also have it support returning the total number of measurements, so we don't have to do 2 requests.
  • Link the network pages from other pages. We should add links to the network page in every measurement page as well as the country pages. This way they will be discoverable and exposed to end users.
  • Create an index/search of all networks. We should have some way to do discovery and search of all the measured countries. This should go hand in hand with rethinking of the country page listing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create network centric views for Explorer
2 participants