Skip to content

feat: fine-grained telemetry with per-stage latency tracking#20

Merged
ZhaoChaoqun merged 2 commits into
mainfrom
feat/telemetry-v1-fine-grained
Mar 23, 2026
Merged

feat: fine-grained telemetry with per-stage latency tracking#20
ZhaoChaoqun merged 2 commits into
mainfrom
feat/telemetry-v1-fine-grained

Conversation

@ZhaoChaoqun

Copy link
Copy Markdown
Owner

Summary

  • Integrate TelemetryDeck SDK and implement v1 analytics optimized for free-tier quota
  • Add per-stage precise millisecond latency tracking to PostProcessing.Completed (TermNorm, ITN, CSC, Punctuation, CloudRewrite)
  • Add ASR.FlushCompleted and Model.LoadCompleted new events
  • Each recording session triggers max 4 events to conserve free TelemetryDeck quota

Changes

File Change
AnalyticsService.swift New file — TelemetryDeck wrapper with elapsedMs() helper + bucket functions
PostProcessingPipeline.swift Per-stage ContinuousClock timing, precise ms in event params
RecordingManager.swift ASR.FlushCompleted event + Session.Completed with duration tracking
ASREngineFactory.swift Model.LoadCompleted event with load time and success/failure
TypelessApp.swift Initialize analytics + App.Launched event
SettingsGeneralView.swift Analytics opt-out toggle in Settings
project.pbxproj TelemetryDeck SPM dependency + AnalyticsService build ref
telemetry-schema-design.md Updated to v1.1 reflecting actual implementation

Events per recording session (quota budget)

Session.Completed + ASR.FlushCompleted + PostProcessing.Completed + CloudRewrite.Completed = 4 events

Test plan

  • Build succeeds (verified ✅)
  • Launch app → verify App.Launched and Model.LoadCompleted fire in TelemetryDeck dashboard
  • Record a session → verify ASR.FlushCompleted, PostProcessing.Completed, CloudRewrite.Completed, Session.Completed fire
  • Check PostProcessing.Completed contains per-stage latency params (termNormLatencyMs, itnLatencyMs, etc.)
  • Toggle analytics off in Settings → verify no events fire
  • Verify DEBUG builds are tagged as test signals in TelemetryDeck

🤖 Generated with Claude Code

Integrate TelemetryDeck SDK and implement v1 analytics optimized for
free-tier quota (max 4 events per recording session). Key changes:

- Add AnalyticsService with elapsedMs() helper and privacy-safe buckets
- Enhance PostProcessing.Completed with per-stage precise ms latency
  (TermNorm, ITN, CSC, Punctuation, CloudRewrite)
- Add ASR.FlushCompleted event for core ASR flush performance
- Add Model.LoadCompleted event for model initialization tracking
- Add App.Launched and Session.Completed baseline events
- Add analytics opt-out toggle in Settings UI
- Update telemetry schema design doc to v1.1

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread Sources/AnalyticsService.swift
Comment thread Sources/AnalyticsService.swift
Comment thread Sources/PostProcessingPipeline.swift Outdated
Comment thread Sources/AnalyticsService.swift Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Code Review — Linus 风格

Talk is cheap. Show me the code. 好,我看了。

整体来说这个 PR 做的事情是对的——给 ASR pipeline 加遥测追踪,quota 意识也到位(每次录音 4 个事件),代码结构清晰。但有几个问题需要处理。

发现的问题

  1. 文档与代码矛盾AnalyticsService.swift):注释说「All numeric values are bucketed to prevent re-identification」,但 PostProcessing.Completed 直接发送精确 ms 值。你定义了 fineLatencyBucket() 却没用。文档和代码说的不是一回事,这比没有文档还糟糕。

  2. 初始化时序风险AnalyticsService.swift):用户启动时若 analytics 已禁用,TelemetryDeck.initialize() 永远不会被调用。之后 toggle 开启时 setEnabled(true) 才初始化。需要确认 TelemetryDeck SDK 对"未初始化就调 signal"的行为是 safe 的,不然这就是一个 latent crash。

  3. PostProcessingPipeline 里大段 copy-paste:两个分支里的 analytics params 字典几乎一模一样,只有 cscApplied/punctuationApplied 不同。这种重复代码是维护噩梦——应该提取成 helper。

  4. cloudRewriteEnabled 计算过于晦涩:三层否定嵌套 + 每次 track 调用都重新计算。可读性极差。

做得不错的地方

  • ContinuousClock 的使用很规范,比 Date() 差值靠谱得多
  • elapsedMs() 的 attoseconds 转换数学是对的
  • [weak self] 在所有异步闭包里都正确使用了
  • Opt-out 模型 + UI toggle 的用户体验是合理的
  • 事件粒度设计(4 events per session)对免费 quota 很务实

评分

维度 分值 评价
正确性 2.5/3 初始化时序有风险,需要确认 TelemetryDeck SDK 行为
简洁性 1.5/2 PostProcessing 里的 copy-paste 扣分
可读性 1.5/2 cloudRewriteEnabled 那行需要人肉编译,文档与代码不一致
可维护性 1.5/2 重复代码意味着改一个忘另一个
安全性 1/1 没有发送敏感数据,appID 是公开的

总分:8/10 — COMMENT

代码方向正确,问题不阻塞合并但建议修复。特别是文档 vs 代码的矛盾——你的文档承诺了隐私保护(bucketed values),代码却发送精确值,这个要统一。

- Fix #1: Update doc comments to reflect precise ms (not bucketed)
  design decision; remove unused fineLatencyBucket()
- Fix #2: Always initialize TelemetryDeck SDK regardless of enabled
  state, eliminating uninitialized-signal risk on toggle
- Fix #3: Extract trackPostProcessingCompleted() helper to eliminate
  copy-paste analytics params between pipeline branches
- Fix #4: Extract hasCloudRewriteAPIKey computed property for clarity

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Re-review — 修复后代码审查

上一轮提了 4 个问题,全部解决了。逐一确认:

✅ 已修复的问题

  1. 文档与代码矛盾 → 文档注释已更新为「Latency values use precise milliseconds」,与实际行为一致。删除了未使用的 fineLatencyBucket(ms:)。干净。

  2. 初始化时序风险initialize() 现在无条件初始化 TelemetryDeck,track() 里用 guard isEnabled 控制是否发信号。正确的分层——SDK 初始化和业务开关解耦。

  3. PostProcessingPipeline copy-paste → 提取成 trackPostProcessingCompleted() 静态方法,两个分支共用。Single source of truth,改一处就够。

  4. cloudRewriteEnabled 三层否定 → 提取成 hasCloudRewriteAPIKey 计算属性,一眼看懂。

新代码无阻塞问题

  • FSM 状态机保证 sessionStartTime 的 set/clear 路径完整(ready→recording 设置,postProcessing→ready 清除),不存在泄漏
  • 所有 sessionStartTime 访问都在 stateQueue 上,线程安全
  • @AppStorage + setEnabled() 有一次 UserDefaults 的冗余写入,但 setEnabled 的 logger 副作用是有用的,可以接受
  • ContinuousClock 用法规范,elapsedMs 的 attoseconds 算术正确

小提醒(不阻塞)

设计文档 telemetry-schema-design.md 底部「修改的文件清单」还提到 fineLatencyBucket(ms:),但该函数已被删除。下次顺手更新一下。

评分

维度 分值 评价
正确性 3/3 初始化时序已修复,状态机路径完整
简洁性 2/2 重复代码已提取,无冗余抽象
可读性 2/2 命名清晰,文档与代码一致
可维护性 2/2 单一职责,改一处即可
安全性 1/1 无敏感数据泄露

总分:10/10 — APPROVE 🚀

代码干净利落,所有反馈都正确修复了。Talk is cheap, but this code speaks for itself. 合并吧。

@ZhaoChaoqun ZhaoChaoqun merged commit b82d341 into main Mar 23, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant