-
Notifications
You must be signed in to change notification settings - Fork 2
Add API spec for logger client integration into cdp-client #25
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
Open
stefanrammo
wants to merge
1
commit into
main
Choose a base branch
from
spec/logger-integration
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| # JavaScript CDP Client: Logger Integration — API Spec | ||
|
|
||
| ## Problem | ||
|
|
||
| Connecting to a CDP Logger requires manual discovery boilerplate: | ||
|
|
||
| ```javascript | ||
| const client = new studio.api.Client('host:7689', listener); | ||
| client.find("App.CDPLogger").then(logger => { | ||
| logger.subscribeToChildValues("ServerPort", port => { | ||
| const loggerClient = new cdplogger.Client("host:" + port); | ||
| // Now use loggerClient separately... | ||
| }); | ||
| }); | ||
| ``` | ||
|
|
||
| This requires knowing the logger node path, the `ServerPort` child convention, extracting the hostname, and managing two independent client lifecycles. The direct WebSocket connection bypasses StudioAPI authentication and is blocked by browsers when the page is served over HTTPS. The logger client is also a separate npm package (`cdplogger-client`) with its own protobuf schema. | ||
|
|
||
| ## Proposed API Changes | ||
|
|
||
| ### 1. Bundle logger client into `cdp-client` | ||
|
|
||
| The logger client code moves into the `cdp-client` package. Direct construction via `new studio.logger.Client(endpoint)` remains available for cases where the endpoint is already known. | ||
|
|
||
| ### 2. `client.logger()` auto-discovers via services | ||
|
|
||
| New method on `studio.api.Client` (CDP 5.1+): | ||
|
|
||
| ```javascript | ||
| const client = new studio.api.Client('host:7689', listener); | ||
| const loggerClient = await client.logger(); | ||
| // loggerClient is ready to use — requestLoggedNodes(), requestDataPoints(), etc. | ||
| ``` | ||
|
|
||
| Both CDPLogger and LogServer register as `websocketproxy` services with `proxy_type: "logserver"` via their shared `ServerRunner` class. The JS client already receives these via `ServicesNotification` and stores them in `availableServices`. | ||
|
|
||
| **Behavior:** | ||
|
|
||
| - `client.logger()` finds the first service with `proxy_type === 'logserver'` in `availableServices`, creates a service transport via `makeServiceTransport(serviceId)`, and passes it to the logger client. The logger protocol (`DBMessaging.Protobuf.Container`) is tunneled through the StudioAPI WebSocket via `ServiceMessage` — no direct connection to the logger port. | ||
| - `client.logger(name)` filters by service name when multiple loggers exist. The service name is the logger's node path (e.g., `'App.CDPLogger'`). | ||
| - An optional `timeout` (milliseconds) can be passed to reject if no matching service appears in time. | ||
| - `client.loggers()` returns `Promise<Array<{name, metadata}>>` with all currently discovered logger services. Resolves after the first `ServicesNotification` response. | ||
| - Returns `Promise<LoggerClient>`. If no matching logger service has been announced yet, the promise waits until one appears via `ServicesNotification`. | ||
|
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. If they wait forever, should have an optional timeout argument. |
||
|
|
||
| **Caching:** | ||
|
|
||
| - Repeated calls return the same cached instance. | ||
| - A cached logger client whose transport has closed is evicted — the next call triggers fresh discovery. | ||
|
|
||
| **Lifecycle:** | ||
|
|
||
| - `client.close()` disconnects all cached logger clients, rejects any pending `client.logger()` promises, and clears the cache. | ||
| - After `client.close()`, calls on a disconnected logger client reject immediately with `"Client is disconnected"`. | ||
| - When the main StudioAPI connection drops, service transports are torn down. | ||
|
|
||
| --- | ||
|
|
||
| ## Use Case Examples | ||
|
|
||
| ### Example 1: Query historical data | ||
|
|
||
| ```javascript | ||
| const studio = require("cdp-client"); | ||
| const client = new studio.api.Client("127.0.0.1:7689"); | ||
|
|
||
| const logger = await client.logger(); | ||
| const limits = await logger.requestLogLimits(); | ||
| const points = await logger.requestDataPoints( | ||
| ['Temperature', 'Pressure'], | ||
| limits.startS, limits.endS, 200 /* max points */, 0 /* no batch limit */ | ||
| ); | ||
| points.forEach(p => { | ||
| const temp = p.value['Temperature']; | ||
| console.log(new Date(p.timestamp * 1000), 'min:', temp.min, 'max:', temp.max); | ||
| }); | ||
| ``` | ||
|
|
||
| ### Example 2: Query events | ||
|
|
||
| ```javascript | ||
| const logger = await client.logger(); | ||
| const events = await logger.requestEvents({ | ||
| limit: 100, | ||
| flags: studio.logger.Client.EventQueryFlags.NewestFirst | ||
| }); | ||
| events.forEach(e => console.log(e.sender, e.data.Text)); | ||
| ``` | ||
|
|
||
| ### Example 3: Multiple loggers — select by name | ||
|
|
||
| ```javascript | ||
| const logger = await client.logger('App.CDPLogger'); | ||
| const logServer = await client.logger('App.MyLogServer'); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Code Organization | ||
|
|
||
| ``` | ||
| JavascriptCDPClient/ | ||
| index.js | ||
| studioapi.proto.js | ||
| logger/ | ||
| logger-client.js | ||
| container-pb.js | ||
| package.json | ||
| ``` | ||
|
|
||
| The logger client constructor is extended to also accept a service transport object. The service transport from `makeServiceTransport` provides the same interface as a WebSocket (`send`, `close`, `onopen`, `onmessage`, `onclose`, `onerror`). | ||
|
|
||
| ## Migration Notes | ||
|
|
||
| No breaking changes. The recommended migration from standalone `cdplogger-client`: | ||
|
|
||
| ```javascript | ||
| // Before (direct connection, no authentication): | ||
| const cdplogger = require("cdplogger-client"); | ||
| const loggerClient = new cdplogger.Client('127.0.0.1:17000'); | ||
|
|
||
| // After (auto-discovery via proxy, with authentication): | ||
| const studio = require("cdp-client"); | ||
| const client = new studio.api.Client("127.0.0.1:7689", listener); | ||
| const loggerClient = await client.logger(); | ||
| ``` | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should also add a method that returns a list of all loggers. It should return when the first ServicesRequest gets a response (I presume the client sets
subscribetotrue, so it always has the current list of services after that)