-
-
Notifications
You must be signed in to change notification settings - Fork 118
feat: Devtools - A separate, lightweight event bus -- utilizing Dispatch (Sync) #950
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
Changes from 4 commits
859468e
1cfd7ef
5fe505c
8dcba1c
0f8cec0
8a7b56e
3cd95e0
be0b792
353fcaa
a5990bb
d37a487
cd9a4e2
75153b1
820a8cc
b6999f7
c8a2c3f
d89ca14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import { type MosaicClient } from './MosaicClient.js'; | |
| import { type SelectionClause } from './SelectionClause.js'; | ||
| import { MaybeArray } from '@uwdata/mosaic-sql'; | ||
| import { Table } from '@uwdata/flechette'; | ||
| import { EventBus, EventType } from './EventBus.js'; | ||
|
|
||
| interface FilterGroupEntry { | ||
| selection: Selection; | ||
|
|
@@ -50,6 +51,7 @@ export class Coordinator { | |
| public clients = new Set<MosaicClient>; | ||
| public filterGroups = new Map<Selection, FilterGroupEntry>; | ||
| protected _logger: Logger = voidLogger(); | ||
| public eventBus: EventBus; | ||
|
|
||
| /** | ||
| * @param db Database connector. Defaults to a web socket connection. | ||
|
|
@@ -77,7 +79,9 @@ export class Coordinator { | |
| consolidate = true, | ||
| preagg = {} | ||
| } = options; | ||
| this.eventBus = new EventBus(); | ||
| this.manager = manager; | ||
| this.manager.eventBus = this.eventBus; | ||
| this.manager.cache(cache); | ||
| this.manager.consolidate(consolidate); | ||
| this.databaseConnector(db); | ||
|
|
@@ -126,6 +130,11 @@ export class Coordinator { | |
| if (arguments.length) { | ||
| this._logger = logger || voidLogger(); | ||
| this.manager.logger(this._logger); | ||
| // subscribe logger to events | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good as a stepping stone but I think we can go ahead and make the logger a separate observer that users need to explicitly add. We can remove the logger entirely now imo. |
||
| this.eventBus.observe(EventType.QueryStart, (event) => this._logger.info('Query started:', event)); | ||
| this.eventBus.observe(EventType.QueryEnd, (event) => this._logger.info('Query ended:', event)); | ||
| this.eventBus.observe(EventType.ClientConnect, (event) => this._logger.info('Client connected:', event)); | ||
| this.eventBus.observe(EventType.Error, (event) => this._logger.error('Error:', event.message)); | ||
| } | ||
| return this._logger!; | ||
| } | ||
|
|
@@ -195,16 +204,18 @@ export class Coordinator { | |
| cache?: boolean; | ||
| persist?: boolean; | ||
| priority?: number; | ||
| clientId?: string; | ||
| [key: string]: unknown; | ||
| } = {} | ||
| ): QueryResult<any> { | ||
| const { | ||
| type = 'arrow', | ||
| cache = true, | ||
| priority = Priority.Normal, | ||
| clientId, | ||
| ...otherOptions | ||
| } = options; | ||
| return this.manager.request({ type, query, cache, options: otherOptions }, priority); | ||
| return this.manager.request({ type, query, cache, options: otherOptions, clientId }, priority); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -246,12 +257,12 @@ export class Coordinator { | |
| priority: number = Priority.Normal | ||
| ): Promise<unknown> { | ||
| client.queryPending(); | ||
| return client._pending = this.query(query, { priority }) | ||
| return client._pending = this.query(query, { priority, clientId: (client as any).id }) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as any is worrying me a bit here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking we get rid of the clientId for now?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's do that |
||
| .then( | ||
| data => client.queryResult(data).update(), | ||
| err => { this._logger?.error(err); client.queryError(err); } | ||
| err => { this.eventBus.emit(EventType.Error, { message: err, timestamp: Date.now() }); client.queryError(err); } | ||
|
domoritz marked this conversation as resolved.
Outdated
|
||
| ) | ||
| .catch(err => this._logger?.error(err)); | ||
| .catch(err => { this.eventBus.emit(EventType.Error, { message: err, timestamp: Date.now() }); }); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -280,6 +291,17 @@ export class Coordinator { | |
| throw new Error('Client already connected.'); | ||
| } | ||
|
|
||
| // assign client id if not present | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of this. What is this needed? Can we add this later?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. understandable, this is only for giving IDs to clients (I did not find a native client-id assignment in the project (let me know if there is one! :) )
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My question is, can we just not have no ids at all. |
||
| if (!(client as any).id) { | ||
| (client as any).id = `client-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; | ||
| } | ||
|
|
||
| // emit ClientConnect | ||
| this.eventBus.emit(EventType.ClientConnect, { | ||
| clientId: (client as any).id, | ||
| timestamp: Date.now() | ||
| }); | ||
|
|
||
| // add client to client set | ||
| clients?.add(client); | ||
|
|
||
|
|
@@ -394,4 +416,4 @@ function updateSelection( | |
| const query = info?.query(active.predicate) ?? client.query(filter); | ||
| return mc.updateClient(client, query); | ||
| })); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { ObserveDispatch } from './util/ObserveDispatch.js'; | ||
|
|
||
| export enum EventType { | ||
| QueryStart = 'query-start', | ||
| QueryEnd = 'query-end', | ||
| ClientConnect = 'client-connect', | ||
| ClientStateChange = 'client-state-change', | ||
| Error = 'error' | ||
| } | ||
|
|
||
| export interface QueryStartEvent { | ||
| query: string; | ||
| materialized: boolean; | ||
| clientId?: string; | ||
| timestamp: number; | ||
|
domoritz marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| export interface QueryEndEvent { | ||
| query: string; | ||
| materialized: boolean; | ||
| clientId?: string; | ||
| timestamp: number; | ||
| } | ||
|
|
||
| export interface ClientConnectEvent { | ||
| clientId: string; | ||
| timestamp: number; | ||
| } | ||
|
|
||
| export interface ErrorEvent { | ||
| message: unknown; | ||
| timestamp: number; | ||
| } | ||
|
|
||
| export class EventBus { | ||
|
domoritz marked this conversation as resolved.
Outdated
|
||
| private dispatch = new ObserveDispatch(); | ||
|
|
||
| observe(type: EventType, callback: (value: any) => void): void { | ||
| this.dispatch.observe(type, callback); | ||
| } | ||
|
|
||
| unobserve(type: EventType, callback: (value: any) => void): void { | ||
| this.dispatch.unobserve(type, callback); | ||
| } | ||
|
|
||
| emit(type: EventType, value: any): void { | ||
| this.dispatch.emit(type, value); | ||
| } | ||
| } | ||
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.
This is where we should add or remove our custom observer that redirects events to console.