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.

Sign up for GitHub

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

Jump to bottom

More options for the ITN uptime analysis. #8

Merged
merged 17 commits into from
Oct 31, 2023
Merged

More options for the ITN uptime analysis. #8

merged 17 commits into from
Oct 31, 2023

Conversation

Sventimir
Copy link
Contributor

We have experienced some problems with the ITN uptime analysis recently. It seems that very often uptime submissins have been coming from fluctuating ports. This might be due to cloud infrastructure used to host those nodes, which created some ephemeral ports for these services. The excat reason is not known. The situation prompted a feature request to the analyser to ignore IP addresses of the nodes and only aggregate submissions by public keys. This improves computed uptimes, all of which turn out to be 100% over the ITN lifetime so far.

Additionally some changes were made to facilitate testing:

  • The program has been decoupled from Google Cloud infrastructure.
  • The time period to analyse is taken from configuration (instead of the Google Cloud spreadsheet)
  • The results are output to standard output as a CSV file.

In the absence of specified time interval, it assumes last full 12-hour period between midnight and noon or between noon and midnight.

Important: Even though any time interval can be specified, data from only the starting day is taken into account.

@Sventimir Sventimir added the enhancement New feature or request label Oct 23, 2023
@Sventimir Sventimir marked this pull request as draft October 24, 2023 06:41
@Sventimir
Copy link
Contributor Author

Sventimir commented Oct 24, 2023 •
edited
Loading

I have just found a bug, which sometimes results in duplicated pks when aggregating over different IPs. This shouldn't happen. Investigating…

@Sventimir Sventimir force-pushed the sventimir/fix-uptime-analysis branch from dd95050 to 91c40b0 Compare October 24, 2023 16:34
@Sventimir Sventimir marked this pull request as ready for review October 25, 2023 07:55
@Sventimir Sventimir force-pushed the sventimir/fix-uptime-analysis branch from f335cf5 to 3ebcc8e Compare October 25, 2023 11:43
Smorci
Smorci suggested changes Oct 25, 2023
View reviewed changes
src/itn_uptime_analyzer/app_config.go Outdated Show resolved Hide resolved
src/itn_uptime_analyzer/identity.go Show resolved Hide resolved
src/itn_uptime_analyzer/README.md Outdated Show resolved Hide resolved
Smorci
Smorci approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@Smorci Smorci left a comment

Choose a reason for hiding this comment

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

Looks good! Great job Marcin!

@Sventimir Sventimir force-pushed the sventimir/fix-uptime-analysis branch 2 times, most recently from bf33cf1 to 30727d0 Compare October 27, 2023 09:43
@Sventimir Sventimir force-pushed the sventimir/fix-uptime-analysis branch from 30727d0 to 1518797 Compare October 27, 2023 09:55
Smorci
Smorci suggested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@Smorci Smorci left a comment

Choose a reason for hiding this comment

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

Logic seems okay, although I had a tough time figuring out what is happening. One liner comments at functions would help, especially in the app_config.go file.

Would you please reflect the changes to the configuration in the README? StdOut, LocalOutput and S3Output?

src/cmd/itn_uptime_analyzer/main_itn_uptime_analyzer.go Outdated Show resolved Hide resolved
src/cmd/itn_uptime_analyzer/main_itn_uptime_analyzer.go Outdated Show resolved Hide resolved
@Sventimir Sventimir force-pushed the sventimir/fix-uptime-analysis branch from 609d334 to 066a5c7 Compare October 31, 2023 07:31
@Sventimir @Smorci
Fix typo.
551eb05 
Co-authored-by: Szekely-Schnedarek Marton <[email protected]>
@Sventimir
Copy link
Contributor Author

Logic seems okay, although I had a tough time figuring out what is happening. One liner comments at functions would help, especially in the app_config.go file.

Would you please reflect the changes to the configuration in the README? StdOut, LocalOutput and S3Output?

Yes, I forgot to update the README. Sorry for that. Will add some comments as well.

Smorci
Smorci approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@Smorci Smorci left a comment

Choose a reason for hiding this comment

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

Awesome work!

@Sventimir Sventimir merged commit 5795571 into main Oct 31, 2023
2 checks passed
@Sventimir Sventimir deleted the sventimir/fix-uptime-analysis branch October 31, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Smorci Smorci Smorci approved these changes

@joaosreis joaosreis Awaiting requested review from joaosreis

@michal0mina michal0mina Awaiting requested review from michal0mina

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Sventimir @Smorci