-
Notifications
You must be signed in to change notification settings - Fork 607
Used Claude code to refactor #7929
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
base: master
Are you sure you want to change the base?
Conversation
@streamer45 @cwarnermm - You can ignore this for the moment. I'm just trying to get it to publish a preview environment so i can look at how things lay out from the Claude Code changes. I'm not sure why I can't get this PR to build a preview |
Ah, I do, @sadohert. It's because it's a fork, not a branch. Only branches (via authorized users) trigger automated previews. For forks, we need to add the |
Newest code from sadohert has been published to preview environment for Git SHA d0ee389 |
@sadohert - Preview now available. |
Newest code from sadohert has been published to preview environment for Git SHA 0a722e5 |
Newest code from sadohert has been published to preview environment for Git SHA 39abca9 |
I'm transitioning these back to markdown. My bad. I thought RST was the standard we were moving to. I'll also resolve the merge conflict. |
a1d3830
to
304864a
Compare
Newest code from sadohert has been published to preview environment for Git SHA 304864a |
Newest code from sadohert has been published to preview environment for Git SHA 52cc483 |
TODOs:
|
700d283
to
ad56133
Compare
Newest code from sadohert has been published to preview environment for Git SHA ad56133 |
Newest code from sadohert has been published to preview environment for Git SHA 4048a34 |
Newest code from sadohert has been published to preview environment for Git SHA 90d72d9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort so far! I gave it a first pass and left some comments (quite a few, to be honest 😅 ) for further tweaking and discussion.
source/configure/calls-deployment.md
Outdated
@@ -88,15 +103,15 @@ This document provides information on how to successfully make the Calls plugin | |||
<td>RTC (Calls plugin or <code>rtcd</code>)</td> | |||
<td>8443</td> | |||
<td>UDP (incoming)</td> | |||
<td>Mattermost clients (Web/Desktop/Mobile)</td> | |||
<td>Mattermost clients (Web/Desktop/Mobile) and calls-offloader</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, it's the jobs spawned by the offloader, such as the recorder or transcriber, that connect back on this port. It’s not necessarily the same instance either; for example, in Kubernetes, they could be scheduled on a different node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will push a change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@streamer45 thoughts on this edit:
source/configure/calls-deployment.md
Outdated
|
||
Media (audio/video) is encrypted using security standards as part of WebRTC. It's mainly a combination of DTLS and SRTP. It's not e2e encrypted in the sense that in the current design all media needs to go through Mattermost which acts as a media router and has complete access to it. Media is then encrypted back to the clients so it's secured during transit. In short: only the participant clients and the Mattermost server have access to unencrypted call data. | ||
**Is calls traffic encrypted?** | ||
Yes, using WebRTC security standards (DTLS/SRTP). Traffic is encrypted in transit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we mean to turn the existing paragraph into a single sentence?
### Browser Console Logs | ||
|
||
Instruct users to check their browser console logs: | ||
|
||
1. **In Chrome/Edge**: | ||
- Press F12 to open Developer Tools | ||
- Go to the Console tab | ||
- Look for errors related to WebRTC, Calls, or media permissions | ||
|
||
2. **Specific patterns to look for**: | ||
|
||
- "getUserMedia" errors (microphone permission issues) | ||
- "ICE connection" failures | ||
- WebSocket connection errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how actionable these would be. Overall, it feels more like internal-facing documentation (for support) rather than something meant for customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this is an admin-facing document. I envision the admin going through early testing (test mode==true) and any helpful client side debug steps would be useful.
Is there anything incorrect in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything incorrect in here?
Not really, but at the same time, these aren't real examples of logs. Would an admin know what a WebSocket connection error refers to? We have at least two WebSocket connections to Mattermost, how would they distinguish between them? What I'm trying to say is that this section could benefit from much more technical investment to become truly valuable (i.e. lower pressure on our support/engineering teams).
To view real-time logs from calls-offloader containers: | ||
|
||
```bash | ||
# Find and follow logs from all calls-related containers | ||
docker ps --format "{{.ID}} {{.Image}}" | grep "calls" | awk '{print $1}' | xargs -I {} docker logs -f {} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to a previous comment, using the dedicated config setting seems simpler.
source/configure/calls-kubernetes.md
Outdated
For Kubernetes deployments, you need to ensure: | ||
|
||
1. UDP traffic is properly routed to RTCD pods (for media) | ||
2. TCP traffic can reach both the Mattermost pods and RTCD pods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads as slightly ambiguous or overly generic. There needs to be a clear connectivity path between Mattermost and RTCD on the API port (TCP 8045).
source/configure/calls-kubernetes.md
Outdated
Recommended annotations for AWS environments: | ||
|
||
```yaml | ||
service.beta.kubernetes.io/aws-load-balancer-backend-protocol: udp | ||
service.beta.kubernetes.io/aws-load-balancer-type: nlb | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these come from official recommendations? Asking because it's the first time I'm seeing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were written by Claude. I don't know if these are valid, and I'm not setup to validate right now. Goign to remove.
- The calls-offloader service runs in a different network segment than clients | ||
- Internal DNS resolution differs from external URLs | ||
- You need to use internal load balancer endpoints for job communication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also quite useful if the Mattermost SiteURL is HTTPs, since it avoids the need to deal with client-side certificates, essentially bypassing all of that complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@streamer45 - Can you elaborate on what you're getting at here?
HTTPS is required for Calls afaik... so there isn't really an "if the Mattermost SiteURL is HTTPS" case, is there?
Is the advantage in the ability to have the Offloader jobs simply connect to the non-TLS endpoints, which is the point behind:
avoids the need to deal with client-side certificates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, a secure connection is required for browsers to initiate calls. The key point here is that we can bypass all of that, including client-side certificates.
Newest code from sadohert has been published to preview environment for Git SHA 0f32682 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass, thanks Stu and Claudio
ice_port_udp = 8443 | ||
ice_address_tcp = "" | ||
ice_port_tcp = 8443 | ||
ice_host_override = "YOUR_RTCD_SERVER_PUBLIC_IP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the best way to indicate that this setting should be set only if you're using ice_host_override
? Or if we can't do that and if we're only putting the defaults, then this should be left blank.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right or wrong, I've mentally come to the conclusion that ice_host_override
will be needed much more often than not.
Is that valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, when using STUN, the override is handled automatically, at least, that was the intention. In practice, though, things are more complex, so explicitly setting the override is the preferred approach in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Met with @cpoile .
@cwarnermm - Do you know if there's a way we could adapt this part of the docs to pull the active version of the sample config file directly into the docs for the reader?
ice_host_override = "YOUR_RTCD_SERVER_PUBLIC_IP" | ||
|
||
# UDP port range for WebRTC connections | ||
ice.port_range.min = 9000 | ||
ice.port_range.max = 10000 | ||
|
||
# STUN/TURN server configuration | ||
ice_servers = [ | ||
{ urls = ["stun:stun.global.calls.mattermost.com:3478"] } | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just keep this to be the defaults, and put a big "NOTE" at the start that you should look at the config.sample.toml
for in-place documentation for each of the settings?
--set ingress.enabled=true \ | ||
--set ingress.host=rtcd.example.com \ | ||
--set service.annotations."service\\.beta\\.kubernetes\\.io/aws-load-balancer-backend-protocol"=udp \ | ||
--set rtcd.ice.hostOverride=rtcd.example.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? (the ice hostOverride=rtcd.example.com bit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so because the override would be set through the RTCD_RTC_ICEHOSTOVERRIDE
env variable.
### RTCD Configuration File | ||
|
||
The RTCD service uses a TOML configuration file. Here's an example with commonly used settings: | ||
|
||
```toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% better to just link. High potential to forget to update this.
@streamer45 What do you think about the example toml file above in the systemd installation section?
|
||
### Required Mattermost Server Configuration | ||
|
||
When using RTCD, you must configure the Mattermost server's CORS settings to allow proper communication between the server and the RTCD service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, probably hallucinated. It does need to be in the offloader setup file though (if it isn't already -- haven't reached it yet)
**Possible causes and solutions**: | ||
|
||
1. **Network connectivity issues**: | ||
- Verify that UDP port 8443 (or your configured port) is open between clients and RTCD servers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify that UDP port 8443 (or your configured port) is open between clients and RTCD servers | |
- Verify that UDP port 8443 (or your configured port) is open between clients (web, desktop, mobile) and RTCD servers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense... should we add calls-offloader
to that list of client examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be thorough yes, as they need to connect to RTCD, same as any other client.
|
||
1. **HTTP API connectivity test**: | ||
|
||
Test if the RTCD API is reachable: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to specify that this is from the Mattermost server's machine (each one, in an HA setup).
curl http://YOUR_RTCD_SERVER:8045/version | ||
# Example response: {"buildDate":"2025-04-02 21:33","buildVersion":"v1.1.0","buildHash":"7bc1f7a","goVersion":"go1.23.6","goOS":"linux","goArch":"amd64"} ``` | ||
|
||
2. **UDP connectivity test**: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like Claudio mentioned above, this has to be done when RTCD is not running (no service bound to 8443)
Co-authored-by: Christopher Poile <[email protected]>
Co-authored-by: Christopher Poile <[email protected]>
@@ -185,13 +185,25 @@ RTCD is the only officially supported approach for Kubernetes deployments. For d | |||
|
|||
## When to Use RTCD | |||
|
|||
The dedicated RTCD service (available with Enterprise license) is recommended for: | |||
This section will help you understand when and why your organization would want to use the dedicated RTCD service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@streamer45 - I believe this essentially restores all the previous content on "Why" that we had before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Caveat" edits
@@ -219,15 +231,16 @@ Mattermost Calls can function in air-gapped environments. Exposing Calls to the | |||
Calls performance primarily depends on: | |||
|
|||
- **CPU resources**: More participants require more processing power | |||
- **Network bandwidth**: Both incoming and outgoing traffic increases with participant count | |||
- **Network bandwidth**: Both incoming and outgoing traffic increases with participant count. Due to the nature of the service, the bottleneck is always going to be the outgoing/egress path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Bottleneck"
Edited based on @streamer45 feedback here - #7929 (comment)
- **Active speakers**: Unmuted participants require significantly more resources | ||
- **Presenters**: Screen sharing participants require even more resources than active speakers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing from this comment
How's this extra bullet @streamer45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Co-authored-by: Christopher Poile <[email protected]>
Co-authored-by: Claudio Costa <[email protected]>
Sub pages were added. Claude seemed to expand on the documentation for calls configuration, including deployment, metrics monitoring, RTC setup, and troubleshooting.