[RUM-14619] Add AddViewLoadingTime telemetry to browser schema#352
Conversation
9d9df43 to
e3ec305
Compare
e3ec305 to
394818a
Compare
c05a910 to
af6039a
Compare
af6039a to
be67b90
Compare
|
The approach looks good to me however:
|
|
💭 thought: Looking at your implementation, I think it could be interesting to introduce an |
e2a11df to
ae6d7c3
Compare
@bcaudan this is done, thanks |
@bcaudan I feel it's a bit overkill to be completely honest. We didn't have this intent captured in the telemetry coming from our mobile SDKs, so I'd rather keep it simple. It's an experimental feature in our Browser SDK anyway. Do you have a strong opinion here? Or was this more of a idea? |
mormubis
left a comment
There was a problem hiding this comment.
Clean move — the schema diff is minimal and the generated types look exactly right. Adding browser and mobile samples is a nice touch for documenting the difference between the two payloads.
Left a couple of inline questions before approving.
| "no_view": { | ||
| "type": "boolean", | ||
| "description": "Whether the view is not available" | ||
| }, | ||
| "no_active_view": { | ||
| "type": "boolean", | ||
| "description": "Whether the available view is not active" | ||
| }, |
There was a problem hiding this comment.
Quick alignment check: are these tracking call-site options (what the caller explicitly passed) or runtime state (what the environment looked like at call time)? The browser-sdk guidance says addTelemetryUsage should be for static call-site info only and specifically calls out "whether a view was active" and "whether a value was overwritten" as examples of runtime state that shouldn't go there. If overwrite is an explicit option passed to the API, overwritten is fine — but no_view/no_active_view feel more like environmental state. Worth a quick check with the team to make sure we're aligned.
There was a problem hiding this comment.
These are mobile-only fields that track environmental state at call time. Indeed, they don't fit the "call-site info only" principle. That said, they already ship in the mobile SDKs and we're just moving them to the common schema for consolidation, while making them optional so the browser SDK can omit them entirely.
I feel changing the mobile behavior would be out of scope here, but let me know 🙇
Yes, it was just a thought, we could do that later if we feel the need. |
| "required": ["feature", "overwritten"], | ||
| "title": "AddViewLoadingTime", |
There was a problem hiding this comment.
💬 suggestion: I think we could set overwritten as optional as well to not implement it on the browser side for now.
There was a problem hiding this comment.
We already send overwritten on the browser side, it's mapped from the overwrite API option. That said, it currently reflects customer intent rather than actual runtime state (i.e. we send true if the user passed @
overwrite: true, and even if no loading time was previously set). So I agree that the current implementation isn't the best.
I can:
- Make
overwrittenoptional - Change the browser SDK implementation so we trigger the telemetry event with
overwrittenset totrueorfalsebased on the outcome and not the intent - Keep it like this as the browser SDK
setViewLoadingTimeis experimental
Let me know what you would prefer 🙇
There was a problem hiding this comment.
I'd go with 1 as
- I don't think we are interested by monitoring
overwrittenfor now on the browser side, so it does not worth the extra bytes - it can be confusing to use
overwrittento monitoroverwrite
If we later feel that we are interested in either overwritten or overwrite on the browse side, we could easily update the schema and the implementation.
mormubis
left a comment
There was a problem hiding this comment.
Previous comments all addressed. LGTM.
1292d02 to
01bfae7
Compare
What does this PR do?
Adds
AddViewLoadingTimeto the shared telemetry features schema (common-features-schema.json), enabling both Browser and Mobile SDKs to track usage of theaddViewLoadingTimeAPI.Why?
The Browser SDK is adding a new
setViewLoadingTime({ overwrite })API for manually reporting view loading times (DataDog/browser-sdk#4180). This schema change enables telemetry tracking aligned with the mobile SDK's existing pattern.The type is placed in the common schema (not browser-only) because both mobile and browser SDKs share the same
addViewLoadingTimeAPI surface and telemetry fields.Changes
AddViewLoadingTimeentry toschemas/telemetry/usage/common-features-schema.jsonwith:feature: "addViewLoadingTime"(const, required)overwritten: boolean(optional) — whether this call overwrote a previously set loading timeno_view: boolean(optional) — API called with no view tracking (mobile only)no_active_view: boolean(optional) — API called when no view is active (mobile only)AddViewLoadingTimefromschemas/telemetry/usage/mobile-features-schema.json(moved to common)lib/cjs/generated/telemetry.d.tsandlib/esm/generated/telemetry.d.tsWhy are all fields except
featureoptional?no_view/no_active_view: The browser SDK always creates a view oninit(), so these are mobile-only. The browser omits them entirely rather than sendingfalse.overwritten: The browser SDK does not send this field for now. Mobile SDKs already report it. Kept optional so each SDK can adopt it independently.Impact
AddViewLoadingTimetype intelemetryEvent.types.ts