-
Notifications
You must be signed in to change notification settings - Fork 162
✨ Video Quality Monitoring adaptation #3822
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: main
Are you sure you want to change the base?
Conversation
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 48f23b9 | Docs | Was this helpful? Give us feedback! |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
82b4965
to
f91ecbf
Compare
export type RumViewEvent = CommonProperties & | ||
ViewContainerSchema & { | ||
ViewContainerSchema & | ||
StreamSchema & { |
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.
question: Is Stream
supposed to be its own event or part of the RUM View? I'm confused
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 idea is it to be part of the RUM View
|
||
const subscription = lifeCycle.subscribe(LifeCycleEventType.RUM_EVENT_COLLECTED, (event): void => { | ||
if (event.type === 'view' || event.type === 'vital' || !isChildEvent(event)) { | ||
if (event.type === 'view' || event.type === 'vital' || !isChildEvent(event) || ['stream'].includes(event.type)) { |
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.
suggestion: let's keep things uniform, either use event.type === 'stream'
or use a list of event types that are never counted containing view
, vital
and stream
lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, serverRumEvent) | ||
|
||
if (rawRumEvent.type === 'stream') { | ||
const streamEvent = { |
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.
thought: this doesn't look like production ready code..
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.
My knowledge is that the idea is to make it work, and have it tested next week with customers and if no one want's it, to be dropped. That's why I think maybe the integration is not production ready.
But I will move this to a helper function, use the types and unit test it to make it prod ready.
4391415
to
d52e7e5
Compare
8bc9e4a
to
03c87eb
Compare
5190b39
to
4af6b65
Compare
Motivation
Video Quality Monitoring initiative plans to reuse view events to send stream information. This PR adds those foundational changes. Additionally, there will be an additional PR in the playground where the VQM plugin will be published.
Changes
This PR adds the
raw
types forstream
that gets transformed in the assembly step to aview
event.Test instructions
This PR is difficult to manually test. Changes are planned for a follow-up PR in the playground where the new VQM plugin uses
stream
events.Checklist