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

feat(cli): add info logs for connected services URLS #6088

Closed
wants to merge 1 commit into from

Conversation

harshal619
Copy link

Motivation

Print URLS in the CLI package for connected services.

Description

Prints info logs to show URLs of connected services

Beacon node

  • execution urls
  • builder urls
  • eth1 provider urls

Validator client

  • beacon node urls

Closes #5990

@harshal619 harshal619 requested a review from a team as a code owner November 5, 2023 20:10
@CLAassistant
Copy link

CLAassistant commented Nov 5, 2023

CLA assistant check
All committers have signed the CLA.

@harshal619 harshal619 changed the title feat(ci): add info logs for connected services URLS feat(cli): add info logs for connected services URLS Nov 7, 2023
@nflaig
Copy link
Member

nflaig commented Nov 8, 2023

Thanks for looking into this @harshal619

Few suggestions

  • would be better to log the URLs where they are used, i.e. when each component is initialized
  • add URLs to log context to be more consistent with other logs like Started REST API server address=http://127.0.0.1:9596
  • single line log should be sufficient, could include count as part of context but I don't think it is required as there are usually just 1-3 urls

Possible solution for this unstable...nflaig/log-connected-service-urls

@harshal619
Copy link
Author

@nflaig Makes sense to log URLs where the component is initialized.
The suggested solution by you looks sufficient.

Anyway, it was a good start for me to look into the code and understand it a little.
should I close this PR? As suggested solution already covers all the required changes.

@nflaig
Copy link
Member

nflaig commented Nov 8, 2023

Anyway, it was a good start for me to look into the code and understand it a little.

Solution looks pretty solid overall and is technically correct, just few nuances to better fit into current logging pattern (logs are kinda subjective and not sure I got it quite right either, definitely needs another review)

should I close this PR? As suggested solution already covers all the required changes.

Sure, might make things easier. I will close this one when I open up the other PR.

@nflaig
Copy link
Member

nflaig commented Nov 8, 2023

@nflaig nflaig closed this Nov 8, 2023
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.

Print out URLs of connected services
3 participants