Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(rtcstats): move conference start time to ljm #2544

Merged
merged 1 commit into from
Jul 10, 2024
Merged

feat(rtcstats): move conference start time to ljm #2544

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion modules/RTCStats/RTCStats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import rtcstatsInit from '@jitsi/rtcstats/rtcstats';
import traceInit from '@jitsi/rtcstats/trace-ws';

import {
CONFERENCE_CREATED_TIMESTAMP,
CONFERENCE_JOINED,
CONFERENCE_LEFT,
CONFERENCE_UNIQUE_ID_SET
Expand All @@ -12,6 +13,7 @@ import JitsiConference from '../../JitsiConference';
import { IRTCStatsConfiguration } from './interfaces';
import { RTC_STATS_PC_EVENT, RTC_STATS_WC_DISCONNECTED } from './RTCStatsEvents';
import EventEmitter from '../util/EventEmitter';
import Settings from '../settings/Settings';

const logger = getLogger(__filename);

Expand Down Expand Up @@ -84,6 +86,16 @@ class RTCStats {
} = {}
} = confConfig;

// The statisticsId, statisticsDisplayName and _statsCurrentId (renamed to displayName) fields
// that are sent through options might be a bit confusing. Depending on the context, they could
// be intermixed inside ljm, for instance _statsCurrentId might refer to the email field which is stored
// in statisticsId or it could have the same value as callStatsUserName.
// The following is the mapping between the fields, and a short explanation of each:
// statisticsId -> email, this is only send by jitsi-meet if enableEmailInStats option is set.
// statisticsDisplayName -> nick, this is only send by jitsi-meet if enableDisplayNameInStats option is set.
// localId, this is the unique id that is used to track users throughout stats.
const localId = Settings?.callStatsUserName ?? '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no unique name is provided, shouldn't we generate sonething random, instead of using the empty string and potentially mixing things up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this would be the place to do that, the callStatsUserName is saved locally and the most common use case for it is to track session originating from the same medium (browser, app, etc).
If we were to generate something unique it won't do us much good as we already have a statsSessionId which is unique per session. Having it empty would at least signal that callStatsUserName isn't properly set in some envs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable!


// Reset the trace module in case it wasn't during the previous conference.
// Closing the underlying websocket connection and deleting the trace obj.
this.reset();
Expand Down Expand Up @@ -125,7 +137,8 @@ class RTCStats {
confName,
displayName,
meetingUniqueId,
isBreakoutRoom
isBreakoutRoom,
localId
}

this.sendIdentity(identityData);
Expand All @@ -139,6 +152,10 @@ class RTCStats {
conference.once(CONFERENCE_LEFT, () => {
this.reset();
});

conference.once(CONFERENCE_CREATED_TIMESTAMP, (timestamp: number) => {
this.sendStatsEntry('conferenceStartTimestamp', null, timestamp);
})
}

/**
Expand Down