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

Fix api listen port config var #1110

Merged
merged 5 commits into from
Mar 4, 2023
Merged

Fix api listen port config var #1110

merged 5 commits into from
Mar 4, 2023

Conversation

agouin
Copy link
Member

@agouin agouin commented Feb 23, 2023

APIListenPort was not being used. Now it will be the debug server address from the config unless a non-empty --debug-addr flag is set.

Copy link
Member

@jtieri jtieri left a comment

Choose a reason for hiding this comment

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

It feels like there is a separation of domains here where a user may want access to debug profiles via pprof or a user may want to be able to scrape metrics from the relayer via prom. Perhaps it doesn't matter that we are serving both of those use cases via one address but i'd be interested to hear @mark-rushakoff's thoughts on this.

cmd/flags.go Outdated
@@ -332,7 +332,7 @@ func dstPortFlag(v *viper.Viper, cmd *cobra.Command) *cobra.Command {
}

func debugServerFlags(v *viper.Viper, cmd *cobra.Command) *cobra.Command {
cmd.Flags().String(flagDebugAddr, defaultDebugAddr, "address to use for debug server. Set empty to disable debug server.")
cmd.Flags().String(flagDebugAddr, "", "address to use for debug server. Set empty to disable debug server.")
Copy link
Member

Choose a reason for hiding this comment

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

With the default behavior changing I think the description here could be improved. Maybe referencing the config field that is being consumed now or something.

@mark-rushakoff
Copy link
Member

I am severely out of touch on the past several months of changes on relayer, so I am missing some context here.

There are slightly different use cases for prometheus metrics and the /debug/pprof endpoint. But in general, I don't see an issue with them being served on the same port. In a maximally locked down environment, they could put a reverse proxy in front of the endpoint if they wanted to be 100% sure the metrics scraper couldn't access the pprof endpoint.

@agouin agouin merged commit b7a4aab into main Mar 4, 2023
@agouin agouin deleted the andrew/fix_api_listen_port branch March 4, 2023 23:03
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.

4 participants