feat: support connection type #15
Conversation
docs/guides/publishing.md
Outdated
| @@ -93,15 +93,15 @@ The above response model is self-explanatory. Clients can choose to retry for er | |||
|
|
|||
| ## Headers | |||
|
|
|||
| Raccoon service accepts headers to identify a user connection uniquely. The header name is made configurable as it enables clients to specify a header name that works for them. For, e.g. for a mobile app having a request header as `X-User-id` which identifies the user \(client\) connecting to Raccoon, can configure Raccoon service with the config set as below `SERVER_WEBSOCKET_CONN_UNIQ_ID_HEADER=X-User-id` | |||
| Raccoon service accepts headers to identify a user connection uniquely. The header name is made configurable as it enables clients to specify a header name that works for them. For, e.g. for a mobile app having a request header as `X-User-ID` which identifies the user \(client\) connecting to Raccoon, can configure Raccoon service with the config set as below `SERVER_WEBSOCKET_CONN_ID_HEADER=X-User-ID` | |||
There was a problem hiding this comment.
Can we include the other group parameter and explain clearly, also point it as optional
websocket/connection/upgrader.go
Outdated
| func (u *Upgrader) Upgrade(w http.ResponseWriter, r *http.Request) (Conn, error) { | ||
| identifier := Identifer{ | ||
| ID: r.Header.Get(u.connIDHeader), | ||
| // If connGroupHeader is empty string. By default, it will always have an empty string as Group. This means uniqueness only depends on ID. |
There was a problem hiding this comment.
If the default value is empty how does it work with dashboards where the metrics are expected to be grouped by conn_group?
Could we supply a default value by default when the config is loaded? (set this to say user)?
This way it is forced for someone to set it. wdyt?
There was a problem hiding this comment.
It simply will ignore the tag. No need to group if metrics don't have groups. Default value on tag value? IMO default tag value is confusing. Better not showing it at all if it's not there.
There was a problem hiding this comment.
Resolving this since it's addressed per how we should handle default in the other section.
|
|
||
| conn, err := u.gorillaUg.Upgrade(w, r, nil) | ||
| if err != nil { | ||
| metrics.Increment("user_connection_failure_total", fmt.Sprintf("reason=ugfailure,conn_group=%s", identifier.Group)) |
There was a problem hiding this comment.
We can't choose this type in dashboard if we have a default tag as empty right?
There was a problem hiding this comment.
If it's not there then it's not available. Why would anyone want to select it if it's not there?
There was a problem hiding this comment.
Let's say:
x-1 version of deployment where there was no conn group
x version of deployment where there is conn_group and now the value is 'web'
On dashboard the grouped by has only 'web' ==> how would you select metrics based on empty (no conn group) pre x deployment?
Technically allowing default as "" values will see this problem for new deployments of raccoon. For upgrade from x-1 to x we have the problem listed above.
There was a problem hiding this comment.
In that case, I'll have --default-- as default group to differentiate it with user-defined group header value. I'll also add the identifier factory back for testability. WDYT?
There was a problem hiding this comment.
Like we discussed in the group: There should be a group_default configurable variable whose default value is --default--. This will enable users provide/override this default thus enforcing a cleaner solution and avoids a situation that leads to what is discussed in the comments.
We also need to clarify clearly as part of the architecture docs in how this default_group works and also cite it as a configurable option as part of the production checks section.
websocket/connection/upgrader.go
Outdated
| } | ||
| return Identifer{ | ||
| ID: h.Get(u.connIDHeader), | ||
| // If connGroupHeader is empty string. By default, it will always have an empty string as Group. This means uniqueness only depends on ID. |
There was a problem hiding this comment.
should we reword this now?
NNcrawler
force-pushed
the
support-user-types
branch
from
b110bb9
to
282b878
Compare
No description provided.