[jitsi-dev] [jitsi-videobridge] Generates Statistics asynchronously, separately from publishing them … (#151)


#1

…through StatsTransports so that the delays in one do not prevent the executions of the other. These modifications are in relation to a reported observation that at times the REST API reports the same statistics (including the timestamp) for long periods of time.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/151

-- Commit Summary --

  * Generates Statistics asynchronously, separately from publishing them through StatsTransports so that the delays in one do not prevent the executions of the other.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/RtpChannel.java (40)
    A src/main/java/org/jitsi/videobridge/stats/PeriodicProcessible.java (102)
    M src/main/java/org/jitsi/videobridge/stats/StatsManager.java (569)
    M src/main/java/org/jitsi/videobridge/stats/StatsManagerBundleActivator.java (2)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/151.patch
https://github.com/jitsi/jitsi-videobridge/pull/151.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/151


#2

Looks good to me. I have two non-blocking remarks:

1. We have at least 2-3 places (send-side bandwidth estimation in libjitsi, and the two BitrateController implementation in the bridge) where we could use the PeriodicProcessible. We should move it to a util class in libjitsi, so that it can be reused (and also used without importing impl.neomedia.rtp.remotebitrateestimator.* everywhere). I would also export PeriodicProcessibleWithObject, even though I don't see any other use cases right now, because it is quite general.

2. If my understanding is correct, calling StatsManager#addTransport or StatsManager#addStatistics after StatsManager#start has been called will not have much of an effect, because added Statistics or StatsTransport will not be registered in the executors (unless StatsManager#start is called again, which doesn't see wise). Given the current usage, I don't think this needs fixing, but it is worth mentioning in the documentation. Also I don't see any reason why addTransport and addStatistics should be public, I think they should be made package-private.

@lyubomir please let me know if you would like me to merge this as it is, or wait for updates.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/151#issuecomment-193519767


#3

Thank you! I'd rather address your remarks first.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/151#issuecomment-193813722


#4

The move of RecurringProcessible and related classes requires https://github.com/jitsi/libjitsi/pull/106.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/151#issuecomment-194429140


#5

Merged #151.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/151#event-583941785