fix some telemetry events #2289
Conversation
WalkthroughAdds telemetry collection and plumbing: server populates app:counts events with user-set counts for custom widgets, custom AI presets, and custom settings. Telemetry structs (Go and TypeScript) gain three settings count fields and an app:firstday boolean. RecordTEvent sets app:firstday when TosAgreed is within 24 hours using a cached TosAgreed timestamp helper. Config gains APIs to count custom widgets, AI presets, and settings. Block creation and layout code introduce CreateBlockWithTelemetry and a recordTelemetry flag; starter layouts disable telemetry while normal tab creation enables it. Minor comment and small error-handling tweak included. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/wconfig/settingsconfig.go (3)
748-755: Prefix check for default widgets is fine; consider centralizing the prefixLogic is correct. Small nit: the "defwidget@" prefix appears as a magic string here. Consider a shared const (e.g., DefaultWidgetPrefix = "defwidget@") to avoid drift if the prefix ever changes.
760-767: Custom AI preset filter works; consider future-proofing reserved IDsThe exclusions "ai@global" and "ai@wave" are hard-coded. If more reserved IDs appear, this will under/over-count. Optional: maintain a small reserved set/map or derive from defaults to keep it in sync.
769-788: Docstring overclaims “from the defaults”The implementation counts keys present in the user’s settings.json (excluding telemetry:enabled) without comparing against defaults. If that’s intended, tighten the wording; otherwise, implement an actual diff vs defaults.
Apply this minimal doc fix:
-// CountCustomSettings returns the number of settings the user has customized from the defaults. -// This excludes telemetry:enabled which doesn't count as a customization. +// CountCustomSettings returns the number of keys present in the user's settings.json, +// excluding "telemetry:enabled".Optionally, if you do want a true “diff vs defaults,” I can provide a small helper that loads defaults via ReadDefaultsConfigFile(SettingsFile) and excludes keys whose values match defaults.
pkg/wcore/workspace.go (1)
144-145: Don't silently discard DBFindWindowForWorkspaceId errors (future-proof for real errors).Today, DBFindWindowForWorkspaceId never returns an error, but if it does in the future, we’ll silently ignore it here. Safer to log non-nil errors and proceed.
- windowId, _ := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId) + windowId, findErr := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId) + if findErr != nil { + log.Printf("error finding window for workspace %s: %v\n", workspaceId, findErr) + windowId = "" + }pkg/telemetry/telemetry.go (1)
215-224: Optional: avoid DB hit on every event.If we only need AppFirstDay for a short window, cache a bool in-process (e.g., atomic.Value) until it expires to skip DBGetSingleton on each RecordTEvent call.
pkg/wcore/layout.go (1)
129-156: Good separation: recordTelemetry param makes layout application telemetricly explicit.Using CreateBlockWithTelemetry with a per-call flag keeps UX-driven flows (starter vs user actions) clean. One nit: the debug log prints the full layout, which may include URLs/paths; consider reducing verbosity or guarding by a debug flag.
- log.Printf("ApplyPortableLayout, tabId: %s, layout: %v\n", tabId, layout) + // Consider trimming or gating this log to avoid noisy/PII-ish output. + log.Printf("ApplyPortableLayout, tabId: %s, actions=%d\n", tabId, len(layout))pkg/wcore/block.go (2)
63-105: Safer to capture BlockView before launching goroutine to avoid pointer races.recordBlockCreationTelemetry reads from blockDef.Meta inside a goroutine. While unlikely mutated, capturing the view upfront avoids any future concurrency hazards.
- if recordTelemetry { - go recordBlockCreationTelemetry(blockDef) - } + if recordTelemetry { + blockView := blockDef.Meta.GetString(waveobj.MetaKey_View, "") + if blockView != "" { + go recordBlockCreationTelemetry(blockView) + } + }And update the helper signature accordingly (see next comment).
107-126: Refactor helper to accept the computed view string.Decouples the goroutine from the original BlockDef pointer and simplifies the function.
-func recordBlockCreationTelemetry(blockDef *waveobj.BlockDef) { +func recordBlockCreationTelemetry(blockView string) { defer func() { panichandler.PanicHandler("CreateBlock:telemetry", recover()) }() - blockView := blockDef.Meta.GetString(waveobj.MetaKey_View, "") - if blockView == "" { - return - } tctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second) defer cancelFn() telemetry.UpdateActivity(tctx, wshrpc.ActivityUpdate{ Renderers: map[string]int{blockView: 1}, }) telemetry.RecordTEvent(tctx, &telemetrydata.TEvent{ Event: "action:createblock", Props: telemetrydata.TEventProps{ BlockView: blockView, }, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
cmd/server/main-server.go(1 hunks)frontend/types/gotypes.d.ts(2 hunks)pkg/telemetry/telemetry.go(4 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(2 hunks)pkg/waveobj/wtype.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)pkg/wcore/block.go(2 hunks)pkg/wcore/layout.go(2 hunks)pkg/wcore/workspace.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/server/main-server.go (3)
pkg/wconfig/filewatcher.go (1)
GetWatcher(33-57)pkg/wconfig/settingsconfig.go (1)
CountCustomSettings(771-788)pkg/telemetry/telemetrydata/telemetrydata.go (1)
TEventUserProps(47-64)
pkg/wcore/layout.go (2)
pkg/waveobj/wtype.go (3)
LayoutActionData(204-214)BlockDef(241-244)RuntimeOpts(264-267)pkg/wcore/block.go (1)
CreateBlockWithTelemetry(63-105)
pkg/telemetry/telemetrydata/telemetrydata.go (1)
pkg/telemetry/telemetry.go (1)
AutoUpdateChannel(85-88)
pkg/wcore/block.go (6)
pkg/waveobj/wtype.go (3)
BlockDef(241-244)Block(279-287)Block(289-291)pkg/panichandler/panichandler.go (1)
PanicHandler(25-43)pkg/waveobj/metaconsts.go (1)
MetaKey_View(9-9)pkg/telemetry/telemetry.go (2)
UpdateActivity(269-358)RecordTEvent(203-230)pkg/wshrpc/wshrpctypes.go (1)
ActivityUpdate(713-737)pkg/telemetry/telemetrydata/telemetrydata.go (2)
TEvent(33-45)TEventProps(66-99)
pkg/telemetry/telemetry.go (2)
pkg/wstore/wstore_dbops.go (1)
DBGetSingleton(102-105)pkg/waveobj/wtype.go (2)
Client(125-133)Client(135-137)
pkg/wcore/workspace.go (2)
pkg/wstore/wstore_dbops.go (1)
DBFindWindowForWorkspaceId(396-403)pkg/wcore/layout.go (2)
ApplyPortableLayout(129-156)GetNewTabLayout(82-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (9)
pkg/waveobj/wtype.go (1)
130-131: Clarifying unit on TosAgreed is helpfulExplicitly documenting that TosAgreed is in Unix milliseconds reduces ambiguity when doing time math elsewhere. LGTM.
frontend/types/gotypes.d.ts (2)
827-834: Additions align with backend telemetry payloadsThe new props (settings:customwidgets, settings:customaipresets, settings:customsettings, app:firstday) match the Go JSON tags and look correct. LGTM.
869-872: User props additions look consistentTEventUserProps entries for custom settings/presets/widgets match the server’s $set usage. LGTM.
pkg/telemetry/telemetrydata/telemetrydata.go (2)
61-64: New user-prop fields are well-scopedAdding settings:customwidgets, settings:customaipresets, settings:customsettings as ints with omitempty is appropriate. They’ll drop out when zero. LGTM.
73-74: Confirmed AppFirstDay logic is correct
TheRecordTEventimplementation inpkg/telemetry/telemetry.gosetsAppFirstDayonly when:
client.TosAgreedis non-zero, and- the difference between
time.Now().UnixMilli()andclient.TosAgreedis at most 24 hours (24 × 60 × 60 × 1000 ms).No further changes are required.
pkg/wcore/workspace.go (1)
222-229: AllApplyPortableLayoutcalls updated to include telemetry flag
- Verified that there are no remaining calls to
ApplyPortableLayoutusing the old three-argument signature; the only invocation inpkg/wcore/workspace.gonow has four arguments (ctx, tab.OID, GetNewTabLayout(), true) as intended.LGTM—thanks for making the telemetry flag explicit for user-created layouts.
pkg/telemetry/telemetry.go (1)
21-22: LGTM: waveobj import is required for Client lookup.Necessary for DBGetSingleton[*waveobj.Client]. No issues.
pkg/wcore/layout.go (1)
187-190: LGTM: starter layout correctly disables telemetry.Setting recordTelemetry=false for BootstrapStarterLayout matches the PR’s objective to avoid first-run telemetry noise.
pkg/wcore/block.go (1)
59-61: LGTM: CreateBlock remains as a thin wrapper; defaulting to telemetry-on preserves behavior.Wrapper preserves API while routing to the new function. Good for incremental adoption.
| fullConfig := wconfig.GetWatcher().GetFullConfig() | ||
| customWidgets := fullConfig.CountCustomWidgets() | ||
| customAIPresets := fullConfig.CountCustomAIPresets() | ||
| customSettings := wconfig.CountCustomSettings() | ||
|
|
||
| props.UserSet = &telemetrydata.TEventUserProps{ | ||
| SettingsCustomWidgets: customWidgets, | ||
| SettingsCustomAIPresets: customAIPresets, | ||
| SettingsCustomSettings: customSettings, | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential nil-pointer if the config watcher failed to initialize
wconfig.GetWatcher() can return nil (e.g., fsnotify initialization failure), leading to a panic on .GetFullConfig(). Add a nil-guard and fall back to wconfig.ReadFullConfig().
Apply this diff:
- fullConfig := wconfig.GetWatcher().GetFullConfig()
- customWidgets := fullConfig.CountCustomWidgets()
- customAIPresets := fullConfig.CountCustomAIPresets()
- customSettings := wconfig.CountCustomSettings()
+ watcher := wconfig.GetWatcher()
+ var fullConfig wconfig.FullConfigType
+ if watcher != nil {
+ fullConfig = watcher.GetFullConfig()
+ } else {
+ // fallback without watcher (e.g., fsnotify unavailable)
+ fullConfig = wconfig.ReadFullConfig()
+ }
+ customWidgets := fullConfig.CountCustomWidgets()
+ customAIPresets := fullConfig.CountCustomAIPresets()
+ customSettings := wconfig.CountCustomSettings()
props.UserSet = &telemetrydata.TEventUserProps{
SettingsCustomWidgets: customWidgets,
SettingsCustomAIPresets: customAIPresets,
SettingsCustomSettings: customSettings,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fullConfig := wconfig.GetWatcher().GetFullConfig() | |
| customWidgets := fullConfig.CountCustomWidgets() | |
| customAIPresets := fullConfig.CountCustomAIPresets() | |
| customSettings := wconfig.CountCustomSettings() | |
| props.UserSet = &telemetrydata.TEventUserProps{ | |
| SettingsCustomWidgets: customWidgets, | |
| SettingsCustomAIPresets: customAIPresets, | |
| SettingsCustomSettings: customSettings, | |
| } | |
| watcher := wconfig.GetWatcher() | |
| var fullConfig wconfig.FullConfigType | |
| if watcher != nil { | |
| fullConfig = watcher.GetFullConfig() | |
| } else { | |
| // fallback without watcher (e.g., fsnotify unavailable) | |
| fullConfig = wconfig.ReadFullConfig() | |
| } | |
| customWidgets := fullConfig.CountCustomWidgets() | |
| customAIPresets := fullConfig.CountCustomAIPresets() | |
| customSettings := wconfig.CountCustomSettings() | |
| props.UserSet = &telemetrydata.TEventUserProps{ | |
| SettingsCustomWidgets: customWidgets, | |
| SettingsCustomAIPresets: customAIPresets, | |
| SettingsCustomSettings: customSettings, | |
| } |
🤖 Prompt for AI Agents
In cmd/server/main-server.go around lines 149-159, guard against a nil
wconfig.GetWatcher() before calling .GetFullConfig(): retrieve watcher :=
wconfig.GetWatcher(); if watcher == nil, call fullConfig :=
wconfig.ReadFullConfig(); otherwise call fullConfig := watcher.GetFullConfig();
then compute customWidgets, customAIPresets and customSettings from that
fullConfig (use fullConfig.CountCustomWidgets(),
fullConfig.CountCustomAIPresets(), fullConfig.CountCustomSettings() or the
appropriate fullConfig methods) and populate props.UserSet as before so the code
won’t panic when the watcher failed to initialize.
| "settings:customwidgets"?: number; | ||
| "settings:customaipresets"?: number; | ||
| "settings:customsettings"?: number; | ||
| "activity:activeminutes"?: number; | ||
| "activity:fgminutes"?: number; | ||
| "activity:openminutes"?: number; | ||
| "app:firstday"?: boolean; | ||
| "action:initiator"?: "keyboard" | "mouse"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure the TS generator is updated, since this file is generated
This file is marked as generated. Please confirm cmd/generate/main-generatets.go (or related generator) includes these new fields to avoid losing them on the next regen.
Run this to verify generator references exist:
🏁 Script executed:
#!/bin/bash
# Look for new telemetry fields in the generator sources
rg -n --hidden -C2 -g 'cmd/generate/**' -e 'settings:customwidgets|settings:customaipresets|settings:customsettings|app:firstday'Length of output: 280
It looks like the original ripgrep glob didn’t match any files. Let’s explicitly locate the TS generator and search it for the new fields:
🏁 Script executed:
#!/bin/bash
# 1. Locate the TypeScript generator file
gen_file=$(fd --hidden --full-path 'main-generatets.go' || echo "")
if [ -z "$gen_file" ]; then
echo "⚠️ Generator file main-generatets.go not found under cmd/generate or elsewhere."
exit 0
fi
echo "🔍 Found generator at: $gen_file"
echo "🔎 Searching for new telemetry fields in the generator…"
# 2. Search for the new telemetry keys
rg -n 'settings:customwidgets|settings:customaipresets|settings:customsettings|activity:activeminutes|activity:fgminutes|activity:openminutes|app:firstday|action:initiator' "$gen_file" || \
echo "❌ No references to the new fields were found in the TS generator."Length of output: 731
TS generator missing new telemetry fields
I verified that cmd/generatets/main-generatets.go does not reference the newly added keys in frontend/types/gotypes.d.ts. Without updating the generator, these fields will be dropped on the next regeneration. Please add support for the following telemetry keys in the TS generator:
settings:customwidgetssettings:customaipresetssettings:customsettingsactivity:activeminutesactivity:fgminutesactivity:openminutesapp:firstdayaction:initiator
🤖 Prompt for AI Agents
In frontend/types/gotypes.d.ts around lines 827-834, the TS generator
cmd/generatets/main-generatets.go is missing the newly added telemetry keys so
they will be dropped on regen; update the generator by adding entries for these
keys to the telemetry/field mapping (or schema list) with the correct types:
settings:customwidgets -> number, settings:customaipresets -> number,
settings:customsettings -> number, activity:activeminutes -> number,
activity:fgminutes -> number, activity:openminutes -> number, app:firstday ->
boolean, action:initiator -> union "keyboard" | "mouse"; after adding those
mappings, run the generator to confirm the keys appear in the produced
frontend/types/gotypes.d.ts.
pkg/telemetry/telemetry.go
Outdated
|
|
||
| // Set AppFirstDay if within first day of TOS agreement | ||
| client, err := wstore.DBGetSingleton[*waveobj.Client](ctx) | ||
| if err == nil && client != nil && client.TosAgreed != 0 { | ||
| now := time.Now().UnixMilli() | ||
| oneDayMs := int64(24 * 60 * 60 * 1000) | ||
| if now-client.TosAgreed <= oneDayMs { | ||
| tevent.Props.AppFirstDay = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix first-day check: negative time skew incorrectly sets app:firstday.
If system clock is behind TosAgreed, now - TosAgreed is negative and currently satisfies <= oneDayMs, incorrectly setting AppFirstDay=true. Guard for non-negative deltas.
- // Set AppFirstDay if within first day of TOS agreement
- client, err := wstore.DBGetSingleton[*waveobj.Client](ctx)
- if err == nil && client != nil && client.TosAgreed != 0 {
- now := time.Now().UnixMilli()
- oneDayMs := int64(24 * 60 * 60 * 1000)
- if now-client.TosAgreed <= oneDayMs {
- tevent.Props.AppFirstDay = true
- }
- }
+ // Set AppFirstDay if within first day of TOS agreement
+ client, err := wstore.DBGetSingleton[*waveobj.Client](ctx)
+ if err == nil && client != nil && client.TosAgreed != 0 {
+ diff := time.Since(time.UnixMilli(client.TosAgreed))
+ if diff >= 0 && diff <= 24*time.Hour {
+ tevent.Props.AppFirstDay = true
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Set AppFirstDay if within first day of TOS agreement | |
| client, err := wstore.DBGetSingleton[*waveobj.Client](ctx) | |
| if err == nil && client != nil && client.TosAgreed != 0 { | |
| now := time.Now().UnixMilli() | |
| oneDayMs := int64(24 * 60 * 60 * 1000) | |
| if now-client.TosAgreed <= oneDayMs { | |
| tevent.Props.AppFirstDay = true | |
| } | |
| } | |
| // Set AppFirstDay if within first day of TOS agreement | |
| client, err := wstore.DBGetSingleton[*waveobj.Client](ctx) | |
| if err == nil && client != nil && client.TosAgreed != 0 { | |
| diff := time.Since(time.UnixMilli(client.TosAgreed)) | |
| if diff >= 0 && diff <= 24*time.Hour { | |
| tevent.Props.AppFirstDay = true | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/telemetry/telemetry.go around lines 215 to 224, the first-day check can
be true when system clock is behind TosAgreed (now-TosAgreed negative); change
the condition to ensure the delta is non-negative and within oneDayMs (e.g.,
require TosAgreed <= now AND now - TosAgreed <= oneDayMs, or explicitly check
delta >= 0 && delta <= oneDayMs) before setting tevent.Props.AppFirstDay = true.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/telemetry/telemetry.go (1)
236-241: Fix first-day check: handle clock skew and don't treat unknown TosAgreed as first-day.
- Negative deltas (system clock behind TosAgreed) currently pass the check and incorrectly set AppFirstDay=true.
- Treating tosAgreedTs == 0 (unknown/unset) as first day will mislabel long-time users who don't have TosAgreed populated.
Apply this safer check:
- // Set AppFirstDay if within first day of TOS agreement - tosAgreedTs := GetTosAgreedTs() - if tosAgreedTs == 0 || (tosAgreedTs != 0 && time.Now().UnixMilli()-tosAgreedTs <= int64(24*60*60*1000)) { - tevent.Props.AppFirstDay = true - } + // Set AppFirstDay only if TosAgreed is known and within 24h (guard against negative deltas) + tosAgreedTs := GetTosAgreedTs(ctx) + if tosAgreedTs > 0 { + if d := time.Since(time.UnixMilli(tosAgreedTs)); d >= 0 && d <= 24*time.Hour { + tevent.Props.AppFirstDay = true + } + }
🧹 Nitpick comments (2)
pkg/telemetry/telemetry.go (2)
31-31: Expose a test-only helper to reset the cached TosAgreed timestamp.Atomic cache is process-wide; tests may leak state across cases. Provide a test-only reset to ensure deterministic tests.
Example (new file pkg/telemetry/telemetry_testhelpers.go):
//go:build test package telemetry func ResetTosAgreedCacheForTest() { cachedTosAgreedTs.Store(0) }
33-48: Add a doc comment for exported GetTosAgreedTs.Public identifiers should have GoDoc. Add a brief comment describing cache semantics and ctx usage (nil => short timeout).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/telemetry/telemetry.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/telemetry/telemetry.go (2)
pkg/wstore/wstore_dbops.go (1)
DBGetSingleton(102-105)pkg/waveobj/wtype.go (2)
Client(125-133)Client(135-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build Docsite
- GitHub Check: merge-gatekeeper
🔇 Additional comments (2)
pkg/telemetry/telemetry.go (2)
242-246: Confirm whether AppFirstDay should be persisted on app:activity events.RecordTEvent sets tevent.Props.AppFirstDay, but updateActivityTEvent merges only minute counters (mergeActivity) before storing, so AppFirstDay may be dropped for app:activity rows. If downstream expects AppFirstDay on app:activity, consider carrying it through (e.g., OR it into curActivity before writing) or emitting a separate lightweight event when true.
159-159: No-op formatting change.Blank line changes; nothing to review.
Also applies to: 175-175
| func GetTosAgreedTs() int64 { | ||
| cached := cachedTosAgreedTs.Load() | ||
| if cached != 0 { | ||
| return cached | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
| defer cancel() | ||
| client, err := wstore.DBGetSingleton[*waveobj.Client](ctx) | ||
| if err != nil || client == nil || client.TosAgreed == 0 { | ||
| return 0 | ||
| } | ||
|
|
||
| cachedTosAgreedTs.Store(client.TosAgreed) | ||
| return client.TosAgreed | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make GetTosAgreedTs context-aware and avoid unconditional background work.
Use the caller's context so RecordTEvent's 2s budget is respected and cancellations propagate. Keep a bounded timeout only when ctx is nil.
-func GetTosAgreedTs() int64 {
+// GetTosAgreedTs returns the cached TosAgreed timestamp (unix ms) if available.
+// If not cached, it reads it from the store using the provided context. If ctx is nil,
+// a short timeout is used to avoid blocking telemetry paths.
+func GetTosAgreedTs(ctx context.Context) int64 {
cached := cachedTosAgreedTs.Load()
if cached != 0 {
return cached
}
- ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
- defer cancel()
+ if ctx == nil {
+ var cancel context.CancelFunc
+ ctx, cancel = context.WithTimeout(context.Background(), 2*time.Second)
+ defer cancel()
+ }
client, err := wstore.DBGetSingleton[*waveobj.Client](ctx)
if err != nil || client == nil || client.TosAgreed == 0 {
return 0
}
cachedTosAgreedTs.Store(client.TosAgreed)
return client.TosAgreed
}Follow-up: update the sole call site in RecordTEvent (see separate comment) to pass ctx.
Run this to confirm no other call sites exist and the signature update compiles cleanly:
🏁 Script executed:
#!/bin/bash
# Find calls to GetTosAgreedTs (old signature) that need ctx
rg -nP '\bGetTosAgreedTs\s*\(' -C2Length of output: 696
Make GetTosAgreedTs context-aware and update its sole call site
To ensure timeouts and cancellations propagate from the caller, change GetTosAgreedTs to accept a context.Context and only synthesize a background timeout when none is provided. This is a breaking signature change—update the one call site in RecordTEvent to pass its ctx.
• File pkg/telemetry/telemetry.go, lines 33–48:
-func GetTosAgreedTs() int64 {
+// GetTosAgreedTs returns the cached TosAgreed timestamp (unix ms) if available.
// If not cached, it reads it from the store using the provided context.
// If ctx is nil, a 2s background timeout is used.
+func GetTosAgreedTs(ctx context.Context) int64 {
cached := cachedTosAgreedTs.Load()
if cached != 0 {
return cached
}
- ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
- defer cancel()
+ if ctx == nil {
+ var cancel context.CancelFunc
+ ctx, cancel = context.WithTimeout(context.Background(), 2*time.Second)
+ defer cancel()
+ }
client, err := wstore.DBGetSingleton[*waveobj.Client](ctx)
if err != nil || client == nil || client.TosAgreed == 0 {
return 0
}
cachedTosAgreedTs.Store(client.TosAgreed)
return client.TosAgreed
}• File pkg/telemetry/telemetry.go, around line 237 (in RecordTEvent):
- tosAgreedTs := GetTosAgreedTs()
+ tosAgreedTs := GetTosAgreedTs(ctx)With only this one call site, the change will compile cleanly once updated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func GetTosAgreedTs() int64 { | |
| cached := cachedTosAgreedTs.Load() | |
| if cached != 0 { | |
| return cached | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| client, err := wstore.DBGetSingleton[*waveobj.Client](ctx) | |
| if err != nil || client == nil || client.TosAgreed == 0 { | |
| return 0 | |
| } | |
| cachedTosAgreedTs.Store(client.TosAgreed) | |
| return client.TosAgreed | |
| } | |
| // pkg/telemetry/telemetry.go | |
| // GetTosAgreedTs returns the cached TosAgreed timestamp (unix ms) if available. | |
| // If not cached, it reads it from the store using the provided context. | |
| // If ctx is nil, a 2s background timeout is used. | |
| func GetTosAgreedTs(ctx context.Context) int64 { | |
| cached := cachedTosAgreedTs.Load() | |
| if cached != 0 { | |
| return cached | |
| } | |
| - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| if ctx == nil { | |
| var cancel context.CancelFunc | |
| ctx, cancel = context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| } | |
| client, err := wstore.DBGetSingleton[*waveobj.Client](ctx) | |
| if err != nil || client == nil || client.TosAgreed == 0 { | |
| return 0 | |
| } | |
| cachedTosAgreedTs.Store(client.TosAgreed) | |
| return client.TosAgreed | |
| } |
| func GetTosAgreedTs() int64 { | |
| cached := cachedTosAgreedTs.Load() | |
| if cached != 0 { | |
| return cached | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| client, err := wstore.DBGetSingleton[*waveobj.Client](ctx) | |
| if err != nil || client == nil || client.TosAgreed == 0 { | |
| return 0 | |
| } | |
| cachedTosAgreedTs.Store(client.TosAgreed) | |
| return client.TosAgreed | |
| } | |
| // pkg/telemetry/telemetry.go, inside RecordTEvent (around original line 237) | |
| tosAgreedTs := GetTosAgreedTs(ctx) |
🤖 Prompt for AI Agents
In pkg/telemetry/telemetry.go around lines 33–48 change GetTosAgreedTs to accept
a context.Context parameter (breaking change) and use that context when calling
wstore.DBGetSingleton; if the passed ctx is nil, synthesize a background context
with a 2s timeout (use context.WithTimeout(context.Background(), 2*time.Second)
and defer cancel), otherwise use the caller's ctx directly so
timeouts/cancellations propagate; also update the single caller in
pkg/telemetry/telemetry.go around line 237 (RecordTEvent) to pass its ctx into
the new GetTosAgreedTs(ctx) signature so the code compiles.
No description provided.