Removes telemetry dependence on notifications

src/App.zig

@@ -70,7 +70,7 @@ pub fn init(allocator: Allocator, config: Config) !*App {
     app.config = config;
     app.allocator = allocator;
 
-    app.notification = try Notification.init(allocator, null);
+    app.notification = try Notification.init(allocator);
     errdefer app.notification.deinit();
 
     app.http = try Http.init(allocator, .{
@@ -96,8 +96,6 @@ pub fn init(allocator: Allocator, config: Config) !*App {
     app.telemetry = try Telemetry.init(app, config.run_mode);
     errdefer app.telemetry.deinit();
 
-    try app.telemetry.register(app.notification);
-
     app.arena_pool = ArenaPool.init(allocator);
     errdefer app.arena_pool.deinit();
 

src/Notification.zig

@@ -39,10 +39,9 @@ const List = std.DoublyLinkedList;
 // CDP code registers for the "network_bytes_sent" event, because it needs to
 // send messages to the client when this happens. Our HTTP client could then
 // emit a "network_bytes_sent" message. It would be easy, and it would work.
-// That is, it would work until the Telemetry code makes an HTTP request, and
+// That is, it would work until multiple CDP clients connect, and because
-// because everything's just one big global, that gets picked up by the
+// everything's just one big global, events from one CDP session would be sent
-// registered CDP listener, and the telemetry network activity gets sent to the
+// to all CDP clients.
-// CDP client.
 //
 // To avoid this, one way or another, we need scoping. We could still have
 // a global registry but every "register" and every "emit" has some type of
@@ -50,14 +49,10 @@ const List = std.DoublyLinkedList;
 // between components to share a common scope.
 //
 // Instead, the approach that we take is to have a notification instance per
-// scope. This makes some things harder, but we only plan on having 2
+// CDP connection (BrowserContext). Each CDP connection has its own notification
-// notification instances  at a given time: one in a Browser and one in the App.
+// that is shared across all Sessions (tabs) within that connection. This ensures
-// What about something like Telemetry, which lives outside of a Browser but
+// proper isolation between different CDP clients while allowing a single client
-// still cares about Browser-events (like .page_navigate)? When the Browser
+// to receive events from all its tabs.
-// notification is created, a `notification_created` event is raised in the
-// App's notification, which Telemetry is registered for. This allows Telemetry
-// to register for events in the Browser notification. See the Telemetry's
-// register function.
 const Notification = @This();
 // Every event type (which are hard-coded), has a list of Listeners.
 // When the event happens, we dispatch to those listener.
@@ -85,7 +80,6 @@ const EventListeners = struct {
     http_request_auth_required: List = .{},
     http_response_data: List = .{},
     http_response_header_done: List = .{},
-    notification_created: List = .{},
 };
 
 const Events = union(enum) {
@@ -102,7 +96,6 @@ const Events = union(enum) {
     http_request_done: *const RequestDone,
     http_response_data: *const ResponseData,
     http_response_header_done: *const ResponseHeaderDone,
-    notification_created: *Notification,
 };
 const EventType = std.meta.FieldEnum(Events);
 
@@ -162,12 +155,7 @@ pub const RequestFail = struct {
     err: anyerror,
 };
 
-pub fn init(allocator: Allocator, parent: ?*Notification) !*Notification {
+pub fn init(allocator: Allocator) !*Notification {
-
-    // This is put on the heap because we want to raise a .notification_created
-    // event, so that, something like Telemetry, can receive the
-    // .page_navigate event on all notification instances. That can only work
-    // if we dispatch .notification_created with a *Notification.
     const notification = try allocator.create(Notification);
     errdefer allocator.destroy(notification);
 
@@ -178,10 +166,6 @@ pub fn init(allocator: Allocator, parent: ?*Notification) !*Notification {
         .mem_pool = std.heap.MemoryPool(Listener).init(allocator),
     };
 
-    if (parent) |pn| {
-        pn.dispatch(.notification_created, notification);
-    }
-
     return notification;
 }
 
@@ -313,7 +297,7 @@ const Listener = struct {
 
 const testing = std.testing;
 test "Notification" {
-    var notifier = try Notification.init(testing.allocator, null);
+    var notifier = try Notification.init(testing.allocator);
     defer notifier.deinit();
 
     // noop

src/browser/Browser.zig

@@ -60,7 +60,7 @@ pub fn init(app: *App, opts: InitOpts) !Browser {
     var env = try js.Env.init(app, opts.env);
     errdefer env.deinit();
 
-    const notification = try Notification.init(allocator, app.notification);
+    const notification = try Notification.init(allocator);
     app.http.client.notification = notification;
     app.http.client.next_request_id = 0; // Should we track ids in CDP only?
     errdefer notification.deinit();

src/browser/Page.zig

@@ -495,6 +495,12 @@ pub fn navigate(self: *Page, request_url: [:0]const u8, opts: NavigateOpts) !voi
             .timestamp = timestamp(.monotonic),
         });
 
+        // Record telemetry for navigation
+        self._session.browser.app.telemetry.record(.{ .navigate = .{
+            .tls = false, // about:blank is not TLS
+            .proxy = self._session.browser.app.config.http_proxy != null,
+        } });
+
         self._session.browser.notification.dispatch(.page_navigated, &.{
             .req_id = req_id,
             .opts = .{
@@ -537,6 +543,12 @@ pub fn navigate(self: *Page, request_url: [:0]const u8, opts: NavigateOpts) !voi
         .timestamp = timestamp(.monotonic),
     });
 
+    // Record telemetry for navigation
+    self._session.browser.app.telemetry.record(.{ .navigate = .{
+        .tls = std.ascii.startsWithIgnoreCase(self.url, "https://"),
+        .proxy = self._session.browser.app.config.http_proxy != null,
+    } });
+
     session.navigation._current_navigation_kind = opts.kind;
 
     http_client.request(.{

src/telemetry/telemetry.zig

@@ -5,7 +5,6 @@ const Allocator = std.mem.Allocator;
 
 const log = @import("../log.zig");
 const App = @import("../App.zig");
-const Notification = @import("../Notification.zig");
 
 const uuidv4 = @import("../id.zig").uuidv4;
 const IID_FILE = "iid";
@@ -63,32 +62,6 @@ fn TelemetryT(comptime P: type) type {
                 log.warn(.telemetry, "record error", .{ .err = err, .type = @tagName(std.meta.activeTag(event)) });
             };
         }
-
-        // Called outside of `init` because we need a stable pointer for self.
-        // We care page_navigate events, but those happen on a Browser's
-        // notification. This doesn't exist yet, and there isn't only going to
-        // be 1, browsers come and go.
-        // What we can do is register for the `notification_created` event.
-        // In the callback for that, `onNotificationCreated`, we can then register
-        // for the browser-events that we care about.
-        pub fn register(self: *Self, notification: *Notification) !void {
-            if (self.disabled) {
-                return;
-            }
-            try notification.register(.notification_created, self, onNotificationCreated);
-        }
-
-        fn onNotificationCreated(ctx: *anyopaque, new: *Notification) !void {
-            return new.register(.page_navigate, ctx, onPageNavigate);
-        }
-
-        fn onPageNavigate(ctx: *anyopaque, data: *const Notification.PageNavigate) !void {
-            const self: *Self = @ptrCast(@alignCast(ctx));
-            self.record(.{ .navigate = .{
-                .proxy = false,
-                .tls = std.ascii.startsWithIgnoreCase(data.url, "https://"),
-            } });
-        }
     };
 }