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

Added Prefix-level Hegemony to the UI #429

Closed
wants to merge 12 commits into from

Conversation

roopeshsn
Copy link
Member

@roopeshsn roopeshsn commented Apr 4, 2023

Description

Implemented a new page for "Prefix Report" and added prefix-level hegemony to the UI. The components used for the "Network Report" are reused for the "Prefix Report" implementation.

Though the appropriate changes to identify prefix in the search box is not done. Testing needs to be done after the change.

Motivation and Context

#252

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@roopeshsn roopeshsn marked this pull request as draft April 4, 2023 16:44
@roopeshsn roopeshsn changed the title Implemented a new page for "Prefix Report" Added Prefix-level Hegemony to the UI Apr 4, 2023
Copy link
Member

@romain-fontugne romain-fontugne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have added a few comments. Actually you don't need to wait for the search bar to include prefixes, you can test with a handmade url like this:
http://localhost:8080/ihr/en-us/prefixes/8.8.8.0/24

src/router.js Outdated
@@ -104,6 +105,16 @@ export default new Router({
},
meta: { title: 'Network Report - IHR' },
},
{
name: 'prefixes',
path: `${routerBase}prefixes/:asn?`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For prefixes (e.g. 8.8.8.0/24) we gonna have two parameters the prefix (8.8.8.0) and its length (24).
so maybe the path could be ${routerBase}prefixes/:prefix/:prefix_length?

Copy link
Member Author

@roopeshsn roopeshsn Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we consider the path like this, ${routerBase}prefixes/:host/:prefix_length? or can we consider as a whole ${routerBase}prefixes/:prefix? At present, I am considering the path like this, ${routerBase}prefixes/:prefix?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${routerBase}prefixes/:host/:prefix_length? is the best way I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense.

src/views/charts/layouts.js Show resolved Hide resolved
src/views/charts/PrefixDependenciesChart.vue Outdated Show resolved Hide resolved
@roopeshsn roopeshsn marked this pull request as ready for review April 7, 2023 07:24
@roopeshsn roopeshsn self-assigned this Sep 25, 2023
@roopeshsn roopeshsn linked an issue Sep 25, 2023 that may be closed by this pull request
@roopeshsn
Copy link
Member Author

Hi, @romain-fontugne! We discussed this PR in one of the weekly meetings a long time back. What stopped us from merging this is that the PrefixDependenciesChart is not rendered properly. Here's the behavior right now:

prefix-dependencies-chart

As you see the chart shows no data. This is because the x and y arrays of every trace inside traces is empty.

After debugging I came to know this is because of this if and this if cases inside fetchHegemony method. The if conditions validate as false which means they are not executed. I assume those cases are written in order to construct the traces array (It's hard to understand the logic written to construct the traces without proper context and comments), which then passed to ReactiveChart component to construct a chart.

I think there should be an else case in order to construct traces even if these conditions validate as false, lastDate !== maxXIso and firstDate !== minXIso (the if conditions mentioned above). Whether my explanation is good enough to understand the problem? or correct me if I am wrong. I am also available to join in a call to discuss this (if needed)!

@romain-fontugne
Copy link
Member

I think these 'if' were added to label graphs when there is missing data. This is important for the other AS Hegemony graph where we expect a datapoint every 15min but not really for this plot. Feel free to completely remove this.

@roopeshsn
Copy link
Member Author

roopeshsn commented Nov 29, 2023

I think these 'if' were added to label graphs when there is missing data. This is important for the other AS Hegemony graph where we expect a datapoint every 15min but not really for this plot. Feel free to completely remove this.

But after removing that part of the code, the chart won't be constructed because the traces array is still empty.

@romain-fontugne
Copy link
Member

can you make sure that the call to the API returns some results?

@roopeshsn
Copy link
Member Author

can you make sure that the call to the API returns some results?

Yes
Screenshot from 2023-12-01 18-30-05

@romain-fontugne
Copy link
Member

let's come back to this after the vue3 migration!

@roopeshsn
Copy link
Member Author

let's come back to this after the vue3 migration!

Sure, I thought the same.

@dpgiakatos
Copy link
Member

The migration has been successfully completed and merged into the master branch. Please familiarize yourself with the new code structure before returning to this pull request.

@dpgiakatos dpgiakatos changed the base branch from master to dev February 27, 2024 09:07
@dpgiakatos
Copy link
Member

This PR is outdated (Vue2JS implementation) and requires a lot of changes. Also, it has been inactive for a long period. @romain-fontugne, should we address it instead and close this PR?

@dpgiakatos
Copy link
Member

Closed by #719.

@dpgiakatos dpgiakatos closed this Mar 12, 2024
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.

Add Prefix-level Hegemony to the searchbox and UI
3 participants