Normalize all CLI flags for port numbers

[yurishkuro]

We have an inconsistency where some flags for ports only support the port number as an integer, while others as host:port string (typically defaulted to ":####"). We should probably converge onto the latter format which allows better control over which IP(s) the server will be listening, while also accepting just a number as well.

Example of two types of flags:

--admin-http-port int           The http port for the admin server, including health check, /metrics, etc. (default 14271)
--http-server.host-port string  host:port of the http server (e.g. for /sampling point and /baggageRestrictions endpoint) (default ":5778")

cc @objectiser

[yurishkuro]

Related to #1332

[suprememoocow]

Being able to configure the host for all listeners is a requirement for us at GitLab. Would this be something you'd be willing for us to contribute?

Presumably this could be done by keeping the old options (eg --admin-http-port), deprecated, alongside the new ones (eg --admin-http-host-port), with the new setting taking precedence?

[yurishkuro]

Yes, contributions are most welcome. I would advise smaller PRs for this, one per binary.

[annanay25]

I will pick this up.

[jpkrohling]

@annanay25 could you also check the consistency of the port numbers? Like, is the gRPC port the same for the all-in-one and collector? How about the monitoring, admin and http ports?

[yurishkuro]

The ports are consistent because they are all consolidated in a single constants file.

[jpkrohling]

I'd need to check further, but I think the admin port isn't consistent overall.

[annanay25]

Admin ports are different for each service but still consolidated in one file -

// AgentAdminHTTP is the default admin HTTP port (health check, metrics, etc.)
AgentAdminHTTP = 14271

// CollectorAdminHTTP is the default admin HTTP port (health check, metrics, etc.)
CollectorAdminHTTP = 14269

// QueryAdminHTTP is the default admin HTTP port (health check, metrics, etc.)
QueryAdminHTTP = 16687

[objectiser]

Wouldn't it be better to just have the single admin port regardless of executable - if yes, then would suggest 14269, as also used in all-in-one.

[yurishkuro]

It would probably be better, but not backwards compatible.

[objectiser]

Yeah, we would need to deprecate the other ports.

[galileo-pkm]

As discussed in #2120
--bind-to-ip flag would be useful in addition to host:port.
With that, one could bind to a list of IPs without changing the default ports.

[ohdearaugustin]

Can you please reopen this issue. The PR doesn't include the Option for --query.port, which still just uses only a port configuration.

[yurishkuro]

Thanks for flagging it, @ohdearaugustin

[ohdearaugustin]

I'm working on it

[ohdearaugustin]

@yurishkuro I just realized that all the logging is still just outputting the port and not the whole address. Should that maybe also be changed?

Current

{"level":"info","ts":1587314624.3605928,"caller":"app/server.go:145","msg":"Starting CMUX server","port":8080}

or

{"level":"info","ts":1587314637.0620887,"caller":"app/agent.go:89","msg":"shutting down agent's HTTP server","addr":"[::]:5778"}

Suggestion

{"level":"info","ts":1587314958.5147562,"caller":"app/server.go:146","msg":"Starting CMUX server","addr":"127.0.0.1:8080"}

Or is that rather a topic for another Issue? (=

[ohdearaugustin]

Ohh I missed out there is a 3rd version of how it is logged:

{"level":"info","ts":1587315636.6723459,"caller":"server/grpc.go:76","msg":"Starting jaeger-collector gRPC server","grpc.host-port":":14250"}

[yurishkuro]

There may be some remnants of logging just the port number, we can change them to logging the address since most of the flags already accept host-port.