From 9a6c62f02923f9c3b8fa215553fb9cc2e17d05df Mon Sep 17 00:00:00 2001 From: Nikolay Govorov Date: Tue, 3 Feb 2026 16:43:25 +0000 Subject: [PATCH] Removes telemetry dependence on notifications --- src/App.zig | 4 +--- src/Notification.zig | 34 +++++++++------------------------- src/browser/Browser.zig | 2 +- src/browser/Page.zig | 12 ++++++++++++ src/telemetry/telemetry.zig | 27 --------------------------- 5 files changed, 23 insertions(+), 56 deletions(-) diff --git a/src/App.zig b/src/App.zig index fca528cd..0a18457d 100644 --- a/src/App.zig +++ b/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(); diff --git a/src/Notification.zig b/src/Notification.zig index 008c0e08..bc11f38a 100644 --- a/src/Notification.zig +++ b/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 -// because everything's just one big global, that gets picked up by the -// registered CDP listener, and the telemetry network activity gets sent to the -// CDP client. +// That is, it would work until multiple CDP clients connect, and because +// everything's just one big global, events from one CDP session would be sent +// to all CDP clients. // // 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 -// notification instances at a given time: one in a Browser and one in the App. -// What about something like Telemetry, which lives outside of a Browser but -// still cares about Browser-events (like .page_navigate)? When the Browser -// 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. +// CDP connection (BrowserContext). Each CDP connection has its own notification +// that is shared across all Sessions (tabs) within that connection. This ensures +// proper isolation between different CDP clients while allowing a single client +// to receive events from all its tabs. 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 { - - // 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. +pub fn init(allocator: Allocator) !*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 diff --git a/src/browser/Browser.zig b/src/browser/Browser.zig index 42171d6e..4496cace 100644 --- a/src/browser/Browser.zig +++ b/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(); diff --git a/src/browser/Page.zig b/src/browser/Page.zig index c55bf634..5999a019 100644 --- a/src/browser/Page.zig +++ b/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(.{ diff --git a/src/telemetry/telemetry.zig b/src/telemetry/telemetry.zig index cffa8768..ff60ac1f 100644 --- a/src/telemetry/telemetry.zig +++ b/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://"), - } }); - } }; }