feat(DENG-10722): add to/enhance urlbar_events_v2#9246
Conversation
This comment has been minimized.
This comment has been minimized.
…urlbar-events-v2-phase1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
lucia-vargas-a
left a comment
There was a problem hiding this comment.
Comment on descriptions
Co-authored-by: Lucia <30448600+lucia-vargas-a@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Updated the description for exit_type to clarify the exit reasons.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…urlbar-events-v2-phase1
This comment has been minimized.
This comment has been minimized.
|
Claude finished @kbammarito's task in 4m 48s —— View job Claude Code ReviewPosted a formal review with 8 inline comments + a summary on the PR.
Highlights of the review:
|
There was a problem hiding this comment.
Review summary
Nice incremental extension of the urlbar_events_v2 template — the new fields (window_mode, exit_type) and the broader event_name filter line up cleanly with the schema. Two reviewers have already approved; the comments below are mostly polish, with one item I'd flag as a real concern before this lands.
Worth resolving before merge
is_terminalsemantics now disagree with its schema description and withexit_type. With this change,bounceanddisableevents getis_terminal = FALSEand a non-NULLexit_type. The schema still tells users to filter onis_terminal = TRUEto count unique search sessions, so any consumer following that guidance will silently undercount the new exit categories. Either tighten the docstring to make the dropdown-vs-session distinction explicit, or setis_terminal = TRUEfor these events. Inline comment onget_is_terminaland on theis_terminalschema entry.bounceevents whoseengagement_typeis outside the click/enter set fall through toevent_action = NULL. If that combination genuinely never occurs, a comment to that effect would help. Otherwise it's a silent gap. Inline onget_event_action.
Style / readability
selected_resultis read raw at the source thenNULLIF(...,'none')-ed in 4 places downstream while the temp functions still compare= 'none'. Normalizing once inevents_unnestedand switching the function checks toIS NULL/IS NOT NULLwould eliminate the duplication and the mixed convention.get_event_action(event_name, engagement_type, selected_result)is invoked four times with identical args; computing it once in a wrapping CTE would makeengaged_result_type/product_engaged_result_type/annoyance_signal_typeeasier to follow.- Mixed quote styles inside the temp functions (
"click"inget_event_actionvs'click'inget_is_terminal). event_actiondescription picks up an em dash and drops the Oxford "or" — reads a bit awkwardly.view.sqlswitched from an explicit column list to* EXCEPT (...); that's fine if intentional, but the leading--- ... Generated via sql_generators.active_users.comment is now wrong (this is generated bysql_generators.urlbar_events).
Detailed inline comments above. None of these are blockers to me, but the is_terminal ambiguity in particular is worth pinning down before downstream dashboards start querying the new exit events.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Integration report for "feat(DENG-10722): add to/enhance urlbar_events_v2"
|
Description
This PR adds/updates functions and fields and updates the
schema.yamlaccordingly:window_modebounceanddisableevent_nameexit_typefield to classify theseRelated Tickets & Documents
Reviewer, please follow this checklist