Skip to content

Commit 116851b

Browse files
archandattaarchandatta
committed
review: address browser telemetry code review feedback
1 parent 9e4ef95 commit 116851b

3 files changed

Lines changed: 163 additions & 28 deletions

File tree

‎README.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,8 @@ Commands with JSON output support:
211211
- `--start-url <url>` - Initial page to open on launch
212212
- `--pool-id <id>` - Acquire a browser from the specified pool (mutually exclusive with --pool-name; ignores other session flags)
213213
- `--pool-name <name>` - Acquire a browser from the pool name (mutually exclusive with --pool-id; ignores other session flags)
214-
- `--telemetry` - Enable telemetry capture on the new session (`enabled: true`)
214+
- `--telemetry` - Enable telemetry for all categories (`enabled: true`)
215+
- `--telemetry=<assignments>` - Per-category config at create time, e.g. `--telemetry=network=on,page=off` (same syntax as `telemetry set`)
215216
- `--output json`, `-o json` - Output raw JSON object
216217
- _Note: When a pool is specified, omit other session configuration flags—pool settings determine profile, proxy, viewport, etc._
217218
- `kernel browsers delete <id>` - Delete a browser
@@ -287,13 +288,12 @@ Commands with JSON output support:
287288
- `-o, --output json` - Output raw API response
288289
- `kernel browsers telemetry stop <id>` - Stop telemetry capture (`enabled: false`)
289290
- `-o, --output json` - Output raw API response
290-
- `kernel browsers telemetry set <id>` - Set per-category telemetry config
291-
- `--categories <assignments>` - Comma-separated `name=on|off` pairs, e.g. `network=on,page=off` (console, interaction, network, page)
291+
- `kernel browsers telemetry set <id> <name=on|off>...` - Set per-category telemetry config, e.g. `network=on page=off`
292292
- `-o, --output json` - Output raw API response
293293
- `kernel browsers telemetry status <id>` - Show current telemetry configuration
294294
- `-o, --output json` - Output raw JSON telemetry config
295295
- `kernel browsers telemetry stream <id>` - Stream live telemetry events
296-
- `--categories <list>` - Filter by category (console,network,page,interaction,monitor)
296+
- `--categories <list>` - Filter by category (console,network,page,interaction,monitor); `monitor` is always-on and filter-only — cannot be toggled via `telemetry set`
297297
- `--types <list>` - Filter by event type (e.g. network_response,console_error)
298298
- `--seq <n>` - Resume stream from sequence number (Last-Event-ID)
299299
- `-o, --output json` - Output newline-delimited JSON envelopes

‎cmd/browsers.go‎

Lines changed: 57 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -193,8 +193,8 @@ type BrowsersCreateInput struct {
193193
StartURL string
194194
Extensions []string
195195
Viewport string
196-
TelemetryEnabled bool
197-
Output string
196+
Telemetry string
197+
Output string
198198
}
199199

200200
type BrowsersDeleteInput struct {
@@ -459,8 +459,14 @@ func (b BrowsersCmd) Create(ctx context.Context, in BrowsersCreateInput) error {
459459
}
460460
}
461461

462-
if in.TelemetryEnabled {
462+
if in.Telemetry == "all" {
463463
params.Telemetry = kernel.BrowserTelemetryRequestConfigParam{Enabled: kernel.Opt(true)}
464+
} else if in.Telemetry != "" {
465+
p, err := parseTelemetryCategories(in.Telemetry)
466+
if err != nil {
467+
return err
468+
}
469+
params.Telemetry = kernel.BrowserTelemetryRequestConfigParam{Browser: p}
464470
}
465471

466472
browser, err := b.browsers.New(ctx, params)
@@ -726,15 +732,14 @@ func (b BrowsersCmd) TelemetryStop(ctx context.Context, in BrowsersTelemetryStop
726732
return nil
727733
}
728734

729-
func (b BrowsersCmd) TelemetrySet(ctx context.Context, in BrowsersTelemetrySetInput) error {
730-
if in.Output != "" && in.Output != "json" {
731-
return fmt.Errorf("unsupported --output value: use 'json'")
732-
}
735+
// parseTelemetryCategories parses a comma-separated "name=on|off" string into
736+
// a BrowserTelemetryCategoriesConfigParam. Unmentioned categories are omitted.
737+
func parseTelemetryCategories(s string) (kernel.BrowserTelemetryCategoriesConfigParam, error) {
733738
p := kernel.BrowserTelemetryCategoriesConfigParam{}
734-
for _, part := range strings.Split(in.Categories, ",") {
739+
for _, part := range strings.Split(s, ",") {
735740
kv := strings.SplitN(strings.TrimSpace(part), "=", 2)
736741
if len(kv) != 2 {
737-
return fmt.Errorf("invalid category assignment %q: expected name=on or name=off", part)
742+
return p, fmt.Errorf("invalid category assignment %q: expected name=on or name=off", part)
738743
}
739744
name, val := strings.TrimSpace(kv[0]), strings.TrimSpace(kv[1])
740745
var enabled bool
@@ -744,7 +749,7 @@ func (b BrowsersCmd) TelemetrySet(ctx context.Context, in BrowsersTelemetrySetIn
744749
case "off":
745750
enabled = false
746751
default:
747-
return fmt.Errorf("invalid value %q for category %q: must be 'on' or 'off'", val, name)
752+
return p, fmt.Errorf("invalid value %q for category %q: must be 'on' or 'off'", val, name)
748753
}
749754
switch name {
750755
case "console":
@@ -756,9 +761,20 @@ func (b BrowsersCmd) TelemetrySet(ctx context.Context, in BrowsersTelemetrySetIn
756761
case "page":
757762
p.Page = kernel.BrowserTelemetryCategoryConfigParam{Enabled: kernel.Opt(enabled)}
758763
default:
759-
return fmt.Errorf("unknown category %q: must be one of console, interaction, network, page", name)
764+
return p, fmt.Errorf("unknown category %q: must be one of console, interaction, network, page", name)
760765
}
761766
}
767+
return p, nil
768+
}
769+
770+
func (b BrowsersCmd) TelemetrySet(ctx context.Context, in BrowsersTelemetrySetInput) error {
771+
if in.Output != "" && in.Output != "json" {
772+
return fmt.Errorf("unsupported --output value: use 'json'")
773+
}
774+
p, err := parseTelemetryCategories(in.Categories)
775+
if err != nil {
776+
return err
777+
}
762778
res, err := b.browsers.Update(ctx, in.Identifier, kernel.BrowserUpdateParams{
763779
Telemetry: kernel.BrowserTelemetryRequestConfigParam{Browser: p},
764780
})
@@ -818,10 +834,32 @@ func shouldEmit(eventType string, categories, types []string) bool {
818834
return true
819835
}
820836

837+
var knownTelemetryCategories = []string{"console", "network", "page", "interaction", "monitor"}
838+
839+
var knownTelemetryTypes = []string{
840+
"console_log", "console_error",
841+
"network_request", "network_response", "network_loading_failed", "network_idle",
842+
"page_navigation", "page_dom_content_loaded", "page_load", "page_tab_opened",
843+
"page_layout_shift", "page_lcp", "page_layout_settled", "page_navigation_settled",
844+
"interaction_click", "interaction_key", "interaction_scroll_settled",
845+
"monitor_screenshot", "monitor_disconnected", "monitor_reconnected",
846+
"monitor_reconnect_failed", "monitor_init_failed",
847+
}
848+
821849
func (b BrowsersCmd) TelemetryStream(ctx context.Context, in BrowsersTelemetryStreamInput) error {
822850
if in.Output != "" && in.Output != "json" {
823851
return fmt.Errorf("unsupported --output value: use 'json'")
824852
}
853+
for _, c := range in.Categories {
854+
if !slices.Contains(knownTelemetryCategories, c) {
855+
return fmt.Errorf("unknown category %q: must be one of %s", c, strings.Join(knownTelemetryCategories, ", "))
856+
}
857+
}
858+
for _, t := range in.Types {
859+
if !slices.Contains(knownTelemetryTypes, t) {
860+
pterm.Warning.Printf("unrecognized event type %q — no events will match\n", t)
861+
}
862+
}
825863
br, err := b.browsers.Get(ctx, in.Identifier, kernel.BrowserGetParams{})
826864
if err != nil {
827865
return util.CleanedUpSdkError{Err: err}
@@ -2479,17 +2517,15 @@ func init() {
24792517
// telemetry
24802518
telemetryRoot := &cobra.Command{Use: "telemetry", Short: "Browser telemetry operations"}
24812519
telemetryStream := &cobra.Command{Use: "stream <id>", Short: "Stream live telemetry events", Args: cobra.ExactArgs(1), RunE: runBrowsersTelemetryStream}
2482-
telemetryStream.Flags().StringSlice("categories", []string{}, "Filter by category (console,network,page,interaction,monitor)")
2520+
telemetryStream.Flags().StringSlice("categories", []string{}, "Filter by category (console,network,page,interaction,monitor); monitor is always-on and filter-only — it cannot be toggled via telemetry set")
24832521
telemetryStream.Flags().StringSlice("types", []string{}, "Filter by event type (e.g. network_response,console_error)")
24842522
telemetryStream.Flags().Int64("seq", 0, "Resume stream from sequence number (Last-Event-ID)")
24852523
telemetryStream.Flags().StringP("output", "o", "", "Output format: json for newline-delimited JSON envelopes")
24862524
telemetryStart := &cobra.Command{Use: "start <id>", Short: "Start telemetry capture (enabled: true)", Args: cobra.ExactArgs(1), RunE: runBrowsersTelemetryStart}
24872525
telemetryStart.Flags().StringP("output", "o", "", "Output format: json for raw API response")
24882526
telemetryStop := &cobra.Command{Use: "stop <id>", Short: "Stop telemetry capture (enabled: false)", Args: cobra.ExactArgs(1), RunE: runBrowsersTelemetryStop}
24892527
telemetryStop.Flags().StringP("output", "o", "", "Output format: json for raw API response")
2490-
telemetrySet := &cobra.Command{Use: "set <id>", Short: "Set per-category telemetry config", Args: cobra.ExactArgs(1), RunE: runBrowsersTelemetrySet}
2491-
telemetrySet.Flags().String("categories", "", "Per-category assignments, e.g. network=on,page=off (console, interaction, network, page)")
2492-
_ = telemetrySet.MarkFlagRequired("categories")
2528+
telemetrySet := &cobra.Command{Use: "set <id> <name=on|off>...", Short: "Set per-category telemetry config", Args: cobra.MinimumNArgs(2), RunE: runBrowsersTelemetrySet}
24932529
telemetrySet.Flags().StringP("output", "o", "", "Output format: json for raw API response")
24942530
telemetryStatus := &cobra.Command{Use: "status <id>", Short: "Show current telemetry configuration", Args: cobra.ExactArgs(1), RunE: runBrowsersTelemetryStatus}
24952531
telemetryStatus.Flags().StringP("output", "o", "", "Output format: json for raw API response")
@@ -2725,7 +2761,8 @@ func init() {
27252761
browsersCreateCmd.Flags().Bool("viewport-interactive", false, "Interactively select viewport size from list")
27262762
browsersCreateCmd.Flags().String("pool-id", "", "Browser pool ID to acquire from (mutually exclusive with --pool-name)")
27272763
browsersCreateCmd.Flags().String("pool-name", "", "Browser pool name to acquire from (mutually exclusive with --pool-id)")
2728-
browsersCreateCmd.Flags().Bool("telemetry", false, "Enable telemetry capture on the new session (enabled: true)")
2764+
browsersCreateCmd.Flags().String("telemetry", "", "Enable telemetry: --telemetry for all, --telemetry=network=on,page=off for per-category")
2765+
browsersCreateCmd.Flag("telemetry").NoOptDefVal = "all"
27292766

27302767
// curl
27312768
curlCmd := &cobra.Command{
@@ -2795,7 +2832,7 @@ func runBrowsersCreate(cmd *cobra.Command, args []string) error {
27952832
viewportInteractive, _ := cmd.Flags().GetBool("viewport-interactive")
27962833
poolID, _ := cmd.Flags().GetString("pool-id")
27972834
poolName, _ := cmd.Flags().GetString("pool-name")
2798-
telemetry, _ := cmd.Flags().GetBool("telemetry")
2835+
telemetry, _ := cmd.Flags().GetString("telemetry")
27992836
output, _ := cmd.Flags().GetString("output")
28002837

28012838
if poolID != "" && poolName != "" {
@@ -2905,8 +2942,8 @@ func runBrowsersCreate(cmd *cobra.Command, args []string) error {
29052942
StartURL: startURL,
29062943
Extensions: extensions,
29072944
Viewport: viewport,
2908-
TelemetryEnabled: telemetry,
2909-
Output: output,
2945+
Telemetry: telemetry,
2946+
Output: output,
29102947
}
29112948

29122949
svc := client.Browsers
@@ -3050,9 +3087,8 @@ func runBrowsersTelemetrySet(cmd *cobra.Command, args []string) error {
30503087
client := getKernelClient(cmd)
30513088
svc := client.Browsers
30523089
out, _ := cmd.Flags().GetString("output")
3053-
categories, _ := cmd.Flags().GetString("categories")
30543090
b := BrowsersCmd{browsers: &svc}
3055-
return b.TelemetrySet(cmd.Context(), BrowsersTelemetrySetInput{Identifier: args[0], Categories: categories, Output: out})
3091+
return b.TelemetrySet(cmd.Context(), BrowsersTelemetrySetInput{Identifier: args[0], Categories: strings.Join(args[1:], ","), Output: out})
30563092
}
30573093

30583094
func runBrowsersTelemetryStatus(cmd *cobra.Command, args []string) error {

‎cmd/browsers_test.go‎

Lines changed: 102 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -911,6 +911,12 @@ func (f *FakeProcessService) StdoutStreamStreaming(ctx context.Context, processI
911911
return makeStream([]kernel.BrowserProcessStdoutStreamResponse{{Stream: kernel.BrowserProcessStdoutStreamResponseStreamStdout, DataB64: "aGVsbG8=", Event: ""}, {Event: "exit", ExitCode: 0}})
912912
}
913913

914+
type FakeBrowserTelemetryService struct{}
915+
916+
func (f *FakeBrowserTelemetryService) StreamStreaming(ctx context.Context, id string, query kernel.BrowserTelemetryStreamParams, opts ...option.RequestOption) *ssestream.Stream[kernel.BrowserTelemetryStreamResponse] {
917+
return makeStream([]kernel.BrowserTelemetryStreamResponse{})
918+
}
919+
914920
type FakeLogService struct {
915921
StreamFunc func(ctx context.Context, id string, query kernel.BrowserLogStreamParams, opts ...option.RequestOption) *ssestream.Stream[shared.LogEvent]
916922
}
@@ -1652,13 +1658,34 @@ func TestBrowsersCreate_WithTelemetry(t *testing.T) {
16521658
}}
16531659
b := BrowsersCmd{browsers: fake}
16541660

1655-
err := b.Create(context.Background(), BrowsersCreateInput{TelemetryEnabled: true})
1661+
err := b.Create(context.Background(), BrowsersCreateInput{Telemetry: "all"})
16561662

16571663
assert.NoError(t, err)
16581664
assert.True(t, captured.Telemetry.Enabled.Valid())
16591665
assert.True(t, captured.Telemetry.Enabled.Value)
16601666
}
16611667

1668+
func TestBrowsersCreate_WithTelemetryCategories(t *testing.T) {
1669+
setupStdoutCapture(t)
1670+
var captured kernel.BrowserNewParams
1671+
fake := &FakeBrowsersService{NewFunc: func(ctx context.Context, body kernel.BrowserNewParams, opts ...option.RequestOption) (*kernel.BrowserNewResponse, error) {
1672+
captured = body
1673+
return &kernel.BrowserNewResponse{SessionID: "session123", CdpWsURL: "ws://example"}, nil
1674+
}}
1675+
b := BrowsersCmd{browsers: fake}
1676+
1677+
err := b.Create(context.Background(), BrowsersCreateInput{Telemetry: "network=on,page=off"})
1678+
1679+
assert.NoError(t, err)
1680+
assert.False(t, captured.Telemetry.Enabled.Valid())
1681+
assert.True(t, captured.Telemetry.Browser.Network.Enabled.Valid())
1682+
assert.True(t, captured.Telemetry.Browser.Network.Enabled.Value)
1683+
assert.True(t, captured.Telemetry.Browser.Page.Enabled.Valid())
1684+
assert.False(t, captured.Telemetry.Browser.Page.Enabled.Value)
1685+
assert.False(t, captured.Telemetry.Browser.Console.Enabled.Valid())
1686+
assert.False(t, captured.Telemetry.Browser.Interaction.Enabled.Valid())
1687+
}
1688+
16621689
func TestBrowsersCreate_WithoutTelemetry(t *testing.T) {
16631690
setupStdoutCapture(t)
16641691
var captured kernel.BrowserNewParams
@@ -1842,16 +1869,60 @@ func TestBrowsersTelemetrySet_InvalidValue(t *testing.T) {
18421869
assert.Contains(t, err.Error(), "must be 'on' or 'off'")
18431870
}
18441871

1872+
func TestBrowsersTelemetryStart_UnsupportedOutputErrors(t *testing.T) {
1873+
b := BrowsersCmd{browsers: &FakeBrowsersService{}}
1874+
1875+
err := b.TelemetryStart(context.Background(), BrowsersTelemetryStartInput{Identifier: "session123", Output: "yaml"})
1876+
1877+
assert.Error(t, err)
1878+
assert.Contains(t, err.Error(), "unsupported --output value")
1879+
}
1880+
18451881
func TestBrowsersTelemetryStop_UnsupportedOutputErrors(t *testing.T) {
1846-
fake := &FakeBrowsersService{}
1847-
b := BrowsersCmd{browsers: fake}
1882+
b := BrowsersCmd{browsers: &FakeBrowsersService{}}
18481883

18491884
err := b.TelemetryStop(context.Background(), BrowsersTelemetryStopInput{Identifier: "session123", Output: "yaml"})
18501885

18511886
assert.Error(t, err)
18521887
assert.Contains(t, err.Error(), "unsupported --output value")
18531888
}
18541889

1890+
func TestBrowsersTelemetrySet_UnsupportedOutputErrors(t *testing.T) {
1891+
b := BrowsersCmd{browsers: &FakeBrowsersService{}}
1892+
1893+
err := b.TelemetrySet(context.Background(), BrowsersTelemetrySetInput{Identifier: "session123", Categories: "network=on", Output: "yaml"})
1894+
1895+
assert.Error(t, err)
1896+
assert.Contains(t, err.Error(), "unsupported --output value")
1897+
}
1898+
1899+
func TestBrowsersTelemetryStatus_UnsupportedOutputErrors(t *testing.T) {
1900+
b := BrowsersCmd{browsers: &FakeBrowsersService{}}
1901+
1902+
err := b.TelemetryStatus(context.Background(), BrowsersTelemetryStatusInput{Identifier: "session123", Output: "yaml"})
1903+
1904+
assert.Error(t, err)
1905+
assert.Contains(t, err.Error(), "unsupported --output value")
1906+
}
1907+
1908+
func TestBrowsersTelemetrySet_WhitespaceTolerance(t *testing.T) {
1909+
setupStdoutCapture(t)
1910+
var captured kernel.BrowserUpdateParams
1911+
fake := &FakeBrowsersService{UpdateFunc: func(ctx context.Context, id string, body kernel.BrowserUpdateParams, opts ...option.RequestOption) (*kernel.BrowserUpdateResponse, error) {
1912+
captured = body
1913+
return &kernel.BrowserUpdateResponse{SessionID: id}, nil
1914+
}}
1915+
b := BrowsersCmd{browsers: fake}
1916+
1917+
err := b.TelemetrySet(context.Background(), BrowsersTelemetrySetInput{Identifier: "session123", Categories: " network = on , page = off "})
1918+
1919+
assert.NoError(t, err)
1920+
assert.True(t, captured.Telemetry.Browser.Network.Enabled.Valid())
1921+
assert.True(t, captured.Telemetry.Browser.Network.Enabled.Value)
1922+
assert.True(t, captured.Telemetry.Browser.Page.Enabled.Valid())
1923+
assert.False(t, captured.Telemetry.Browser.Page.Enabled.Value)
1924+
}
1925+
18551926
func TestBrowsersTelemetryStatus_PrintsCategories(t *testing.T) {
18561927
setupStdoutCapture(t)
18571928
fake := &FakeBrowsersService{GetFunc: func(ctx context.Context, id string, query kernel.BrowserGetParams, opts ...option.RequestOption) (*kernel.BrowserGetResponse, error) {
@@ -1879,6 +1950,34 @@ func TestBrowsersTelemetryStatus_PrintsCategories(t *testing.T) {
18791950
assert.Contains(t, out, "page: off")
18801951
}
18811952

1953+
func TestTelemetryStream_UnknownCategoryErrors(t *testing.T) {
1954+
b := BrowsersCmd{browsers: &FakeBrowsersService{}}
1955+
1956+
err := b.TelemetryStream(context.Background(), BrowsersTelemetryStreamInput{
1957+
Identifier: "session123",
1958+
Categories: []string{"invalid"},
1959+
})
1960+
1961+
assert.Error(t, err)
1962+
assert.Contains(t, err.Error(), "unknown category")
1963+
}
1964+
1965+
func TestTelemetryStream_UnknownTypeWarns(t *testing.T) {
1966+
setupStdoutCapture(t)
1967+
fake := &FakeBrowsersService{GetFunc: func(ctx context.Context, id string, query kernel.BrowserGetParams, opts ...option.RequestOption) (*kernel.BrowserGetResponse, error) {
1968+
return &kernel.BrowserGetResponse{SessionID: id}, nil
1969+
}}
1970+
b := BrowsersCmd{browsers: fake, telemetry: &FakeBrowserTelemetryService{}}
1971+
1972+
err := b.TelemetryStream(context.Background(), BrowsersTelemetryStreamInput{
1973+
Identifier: "session123",
1974+
Types: []string{"invalid_type"},
1975+
})
1976+
1977+
assert.NoError(t, err)
1978+
assert.Contains(t, outBuf.String(), "unrecognized event type")
1979+
}
1980+
18821981
func TestEventCategory(t *testing.T) {
18831982
cases := map[string]string{
18841983
"network_response": "network",

0 commit comments

Comments
 (0)