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

Add fetch flags #10

Merged
merged 5 commits into from
May 1, 2023
Merged

Add fetch flags #10

merged 5 commits into from
May 1, 2023

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented May 1, 2023

This PR is in conjunction with #7417 in Erigon.

This creates Fetch Flags button and ui/flags in ui_handler.go to fetch context flags via /debug/metrics/flags in Erigon.

@ensi321 ensi321 changed the title [WIP] Add fetch flags Add fetch flags May 1, 2023
@AlexeyAkhunov
Copy link
Contributor

Thank you! Looks good. Could you also add 2 more things:

  1. section in the documentation README.md about this new Fetch Flags functionality, similar to what is done for other functions
  2. Once the Version is incremented in Erigon's diagnostics/version.go, the diagnostics system may be dealing with version of Erigon node which have Version = 1 and Version = 2. Could you make it so that Fetch Flags only works for Version >= 2 and if the version is too small, display message about it

@ensi321
Copy link
Contributor Author

ensi321 commented May 1, 2023

Thank you! Looks good. Could you also add 2 more things:

  1. section in the documentation README.md about this new Fetch Flags functionality, similar to what is done for other functions
  2. Once the Version is incremented in Erigon's diagnostics/version.go, the diagnostics system may be dealing with version of Erigon node which have Version = 1 and Version = 2. Could you make it so that Fetch Flags only works for Version >= 2 and if the version is too small, display message about it

Regarding your second point, I think this is in the area of API versioning which is tricky and requires some more thought.

For now I have implemented a temporary solution and come up with a better solution later

cmd/flags.go Outdated
flags.Error = result
}
if err := templ.ExecuteTemplate(w, "flags.html", flags); err != nil {
fmt.Fprintf(w, "Executing versions template: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to say flags template

}

if versions.NodeVersion < 2 {
fmt.Fprintf(w, "Flags only support version >= 2. Node version: %d", versions.NodeVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's probably good enough for now, we shall test it

@AlexeyAkhunov
Copy link
Contributor

We can have your temporary solution and add a next improvement idea to what you come up with (I am curious about your idea of how to make it better)

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.

2 participants