-
Notifications
You must be signed in to change notification settings - Fork 6
[VQM] define transition schema #288
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
Conversation
273aa15
to
9d80c99
Compare
3ff03e8
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 is a small typo, but it looks good to me otherwise
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.
LGTM👍
"view": { | ||
"type": "object", | ||
"description": "Attributes of the view's container", | ||
"required": ["id"], | ||
"properties": { | ||
"id": { | ||
"type": "string", | ||
"description": "ID of the parent view", | ||
"pattern": "^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$", | ||
"readOnly": true | ||
} | ||
}, | ||
"readOnly": true | ||
}, | ||
"source": { | ||
"type": "string", | ||
"description": "Source of the parent stream", | ||
"enum": ["android", "ios", "browser", "flutter", "react-native", "roku", "unity", "kotlin-multiplatform"], | ||
"readOnly": true | ||
} |
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: Since we already have view.id
and source
from common schema, do we also need to have them there?
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're right, it's not needed, removing it
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 about source?
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.
yes that one isn't needed either 🤦 removing it...
"container": { | ||
"type": "object", | ||
"description": "Stream Container properties (stream wrapping the current stream)", | ||
"required": ["stream", "source"], | ||
"properties": { | ||
"stream": { | ||
"type": "object", |
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: if source is not needed, we could also remove the container level, no?
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.
yes, i was basing it off the view-container schema, but that wasn't the correct definition to use as an example
in 1992a90 im making it so it's now named _stream-event-schema.json
and it only makes the stream.id
property required
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.
Sorry about the iteration on this one, I'm discovering the topic as I am reviewing.
It seems that we would now have a _stream-event-schema.json
used by view event and a _stream-schema.json
used by transition event.
Depending on how we plan to use those schemas, we could probably make their names and descriptions more explicit.
Happy to chat about it if 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.
Nice!
New event type: transtions
These events are part of the QoE data model definition (Read more about it here https://datadoghq.atlassian.net/wiki/spaces/RUMP/pages/5241110685/QoE+RUM+Payload+Collection)