- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.5k
feat(rtcstats): add rtcstats support #2116
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
        
          
                env.example
              
                Outdated
          
        
      | #RTCSTATS_ENABLED=true | ||
|  | ||
| # Send the console logs to the rtcstats server | ||
| #RTCSTATS_STORE_LOGS=false | 
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.
In this file, RTCSTATS_ENABLED and RTCSTATS_SEND_SDP are both set "true" so when you uncomment them they will be turned on. But RTCSTATS_STORE_LOGS is "false" so if you uncomment it, it won't change anything. I think that this is because you are showing common defaults but the inconsistency seems a little strange to me because the rest of this example file seems to show how to turn things on.
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "admin": "admin" | |||
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 might be better to have this something like "admin": "CHANGE_ME" to make it obvious to someone that they need to set the password to something other than admin here.
| 
 This is fantastic! | 
        
          
                rtcstats/localstack/localstack.yml
              
                Outdated
          
        
      | networks: | ||
| meet.jitsi: | ||
| aliases: | ||
| - localhost.localstack.cloud | 
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.
why are these complex aliases needed?
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've confirmed this also works using the simpler service name localstack, so I will update the configuration to use that method instead.
        
          
                rtcstats/localstack/localstack.yml
              
                Outdated
          
        
      | - ./rtcstats/.env | ||
| environment: | ||
| # LocalStack configuration: https://docs.localstack.cloud/references/configuration/ | ||
| - SERVICES=lambda,iam,logs,s3,dynamodb,firehose | 
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.
are lambda, firehose, iam and logs needed 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.
You are right, we only use S3 and DynamoDB in this setup. The other services like lambda, firehose, iam, and logs are unnecessary. I will remove them.
        
          
                rtcstats/localstack/localstack.yml
              
                Outdated
          
        
      | - AWS_ENDPOINT_URL=http://localhost.localstack.cloud:4566 | ||
| volumes: | ||
| - "./rtcstats/localstack/${LOCALSTACK_VOLUME_DIR:-./volume}:/var/lib/localstack" | ||
| - "/var/run/docker.sock:/var/run/docker.sock" | 
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 does this part here do?
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.
The ./rtcstats/localstack/volume mount was intended for data persistence, but it doesn't seem to be functioning correctly for S3, so I will remove it. The /var/run/docker.sock mount is for services like Lambda. It is not needed here and I will remove that as well.
|  | ||
| From your `docker-jitsi-meet` directory, run the following command to start all services, including Jitsi Meet, rtcstats, and Localstack. | ||
| ```shell | ||
| docker compose -f docker-compose.yml -f rtcstats.yml -f ./rtcstats/localstack/localstack.yml up -d | 
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.
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.
The S3 endpoint is misconfigured. It needs to be http://s3.localhost.localstack.cloud:4566 (with the s3. prefix) so the AWS SDK can make virtual-hosted-style requests. The unknownEndpoint error occurs without it. I'm sorry for leaving this out of the README. The .env file needs these values:
RTCSTATS_S3_ENDPOINT=http://s3.localhost.localstack.cloud:4566
RTCSTATS_DYNAMODB_ENDPOINT=http://localhost.localstack.cloud:4566
I'll commit the documentation fix soon.
| I simplified the localstack.yml file by removing unnecessary settings and also updated the readme to make the setup instructions clearer. | 


This PR integrates
rtcstats-serverandrtc-visualizerinto the docker-compose setup.This PR is currently in Draft mode.It depends on updated Docker images for
jitsi/rtcstats-serverandjitsi/rtc-visualizerthat have not yet been published to the official Docker Hub registry. The current images on Docker Hub are older versions and will not work correctly with these changes.I will mark this PR as "Ready for Review" as soon as the new images are available.