Add protocol to the topology endpoint response #8868
Conversation
freddygv
force-pushed
the
topo-viz-ingress
branch
from
8e0d7db
to
8ccd2cc
Compare
There was a problem hiding this comment.
@banks does it make sense for an ingress gateway to have a single protocol when it can have listeners for TCP and HTTP?
I think for now I'd suggest:
- if it has only listeners of a single protocol, use that protocol
- if it has a mix of
tcpand non-tcp use tcp so we are at least showing all the traffic - we could as an optimisation allow all non-tcp protocols to be treated as
httpso if there is a combination of http, http2, grpc we just return 'http' since they will all have "http ish" metrics as far as we are concerned.
That gets it doing the right thing in the common cases of just one listener or multiple of the same type and is OK in other cases too. Later we could make it optional in the UI or in config I guess.
freddygv
force-pushed
the
topo-viz-protocol
branch
2 times, most recently
from
59ce7ce
to
ea8e8d0
Compare
freddygv
force-pushed
the
topo-viz-protocol
branch
from
9ac4585
to
d2aa6d7
Compare
| kind, ok := req.URL.Query()["kind"] | ||
| if !ok { | ||
| resp.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprint(resp, "Missing service kind") |
There was a problem hiding this comment.
Guarding since we need it for non-typical services
| @@ -519,6 +519,7 @@ type ServiceSpecificRequest struct { | |||
| Datacenter string | |||
| NodeMetaFilters map[string]string | |||
| ServiceName string | |||
| ServiceKind ServiceKind | |||
There was a problem hiding this comment.
Does adding this here in this generic struct impact any other users of this struct?
There was a problem hiding this comment.
I don't think so, callers/APIs that don't need it will just ignore. Like the Connect/Ingress booleans that were added below.
| @@ -2907,6 +2907,36 @@ func serviceGatewayNodes(tx *txn, ws memdb.WatchSet, service string, kind struct | |||
| return maxIdx, ret, nil | |||
| } | |||
|
|
|||
| // metricsProtocolForIngressGateway determines the protocol that should be used when fetching metrics for an ingress gateway | |||
| // Since ingress gateways may have listeners with different protocols, favor capturing all traffic by only returning HTTP | |||
| // when all listeners are HTTP-like. | |||
There was a problem hiding this comment.
(later) Maybe dual-protocol ingress gateways should be rendered as N nodes in the UI instead of trying to mush them into one apples/oranges node like this.
There was a problem hiding this comment.
Yeah that's an option, can bring that up to Hannah and Jasmine
This PR adds the protocol for the queried service, and is resolved from config entries.
@banks does it make sense for an ingress gateway to have a single protocol when it can have listeners for TCP and HTTP?
Need to fix the chained PR dance, only this commit is relevant:
d32fda6(#8868)