fix(sls): add structured flush failure diagnostics#130
Conversation
Classify SLS send failures across AK and webtracking paths, attach safe diagnostic context to alarms, failed logs, and flusher metrics, and document the new fields with focused unit coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
PR adds structured failure diagnostics to SLS flusher: normalized failure_class classification, HTTP status code extraction, redacted reason summary, and endpoint_host (hostname-only) to both FLUSH_SEND_ALARM and failed-log entries. All changes are additive — optional fields in interfaces, backward-compatible alarm aggregation when diagnostic fields are absent.
Key Strengths
- Security:
sanitizeFailureReasonproperly redacts AccessKeyId/accessKeySecret, Bearer/Basic auth, and full URLs; truncated to 240 chars. Tests explicitly verify no sensitive data leaks into alarms or failed-logs. - Backward compat: Legacy alarm dedup key preserved when
failure_class/status_codeabsent;isRetryable()behavior unchanged. - Test coverage: Comprehensive unit tests for
classifySlsFailure,extractEndpointHost,sanitizeFailureReason, alarm splitting, metrics, and flusher integration — including AK and webtracking paths. - Documentation: Both EN and zh-CN docs updated with diagnostic field reference and troubleshooting table.
Observations
- [Info]
isRetryableFailureignores_failureClassparameter and falls back to status-code/text matching — intentional for backward compat, consistent with prior behavior. - [Info] Alarm dedup key now includes
failure_class+status_code, meaning different failure classes for the same endpoint produce separate alarms. This is the correct design for diagnostic clarity.
LGTM. Clean, well-tested, additive change with strong security redaction.
Automated review by github-manager-bot
Extract status and errno details from nested fetch causes so refused and timeout transport errors use network failure classes with retryable diagnostics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
The new commit (544c50b) enhances SLS transport failure classification to handle nested fetch errors. Recursive cause chain traversal, AggregateError (errors[]) support, and errno-based classification are all well-implemented with proper circular-reference protection (seen Set) and depth limits (MAX_ERROR_CAUSE_DEPTH=6). New tests cover nested TypeError classification, deep cause chains, and abort errors. No issues found.
Findings
- [Info] The hardcoded errno values (60, 110, 54, 61, 104, 111) alongside
osConstants.errno.*correctly handle cross-platform differences (macOS vs Linux). Consider adding a brief comment documenting which platform each numeric value corresponds to, for future maintainers.
Automated review by github-manager-bot
|
This PR has conflicts with the git fetch origin
git checkout multica/loo-30-cp1
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
| return 'unknown'; | ||
| } | ||
|
|
||
| function isRetryableFailure( |
There was a problem hiding this comment.
[High] isRetryableFailure 文本兜底判断与 classifyByText 不对齐
影响: classifyByText 将 "serviceunavailable" 和 "internal server error"（含空格）分类为 server_error，但 isRetryableFailure 的文本兜底列表不包含这些模式。无 status code 的服务端错误会被分类为 server_error 但 retryable=false，跳过重试直接失败。
建议: 在 isRetryableFailure 中基于 failureClass 补充显式判定：
if (
failureClass === 'server_error' ||
failureClass === 'quota_throttle'
) return true;这让 retryable 判定直接引用 failureClass 而非重复文本匹配，消除分类层与重试层之间的不一致。
| ); | ||
| } | ||
| await this.persistFailedLogs(endpoint, logGroup, lastErr); | ||
| await this.persistFailedLogs(endpoint, logGroup, diagnostics); |
There was a problem hiding this comment.
[Medium] persistFailedLogs 未被 try-catch 保护，磁盘异常导致计数错误
影响: flushViaAk/postWebtracking 内部 catch 了 SLS 错误并返回 FlushResult，但 persistFailedLogs 若抛异常（如 ENOSPC），整个 promise reject。.catch() handler 把全部 logs.length 计为 outFailed，即使 webtracking 的前几个 chunk 已成功发送。且 classifySlsFailure(err) 将文件系统错误误分类为 SLS 故障。
建议:
try {
await this.persistFailedLogs(endpoint, logGroup, diagnostics);
} catch (e) {
logger.warn('Failed to persist failed logs', { error: String(e) });
}
return { sentEntries: 0, failedEntries: logs.length, lastFailure: diagnostics };同样处理 webtracking 路径（line 425）。
| context?.endpoint_name ?? '', | ||
| context?.failure_class ?? '', | ||
| context?.status_code ?? '', | ||
| ].join('_'); |
There was a problem hiding this comment.
[Medium] 告警 dedup key 含 status_code 可能导致告警碎片化
影响: 旧 key 为 ${type}_${input_name}_${endpoint_name}，同一 endpoint 的失败合并为一条告警。新 key 加入 failure_class + status_code 后，若负载均衡器交替返回 502/503/504，每种 status_code 各产生一条独立告警条目。原本 count=3 的单条告警分裂为 3 条 count=1，下游告警规则若基于 alarm_count 做阈值判断可能漏报。
建议: 考虑从 dedup key 中移除 status_code，仅保留 failure_class；status_code 仍作为 context 字段传递：
const key = [type, context?.input_name ?? '', context?.endpoint_name ?? '', context?.failure_class ?? ''].join('_');
| lower.includes('econnrefused') || | ||
| lower.includes('econnreset') || | ||
| lower.includes('socket hang up') || | ||
| lower.includes('network') || |
There was a problem hiding this comment.
[Medium] classifyByText 中 "network" 匹配过宽
影响: lower.includes('network') 会匹配任何包含 "network" 的错误消息（如 "network policy violation"），误分类为 network_refused。老代码也有宽匹配，但新代码将影响从"仅影响重试"扩大到"影响 failure_class 标签、告警、failed-log、metrics"。
建议: 收紧为 lower.includes('networkerror') || lower.includes('network error') || lower.includes('fetch failed')。
| ); | ||
| } | ||
|
|
||
| function textHasErrno(text: string, errnoValues: Set<string>): boolean { |
There was a problem hiding this comment.
[Medium] textHasErrno 每次调用动态创建 RegExp 对象
影响: 每次调用在循环内为每个 errno 值创建新 RegExp（最多 12 个）。classifyByText 中调用两次，每次错误分类最多创建 24 个 RegExp 对象。频率低但易优化。
建议: 预编译为模块级 Map<string, RegExp>，或合并为单个 alternation regex per set（如 /\berrno[:=]\s*(?:60|-60|110|-110)\b/）。
🔍 Code Review Summary
Lifecycle Verdict
资源释放 FAIL: 总体结论PR 整体架构扎实，additive 改动向后兼容，安全去敏设计（4 层 regex + 白名单提取 + 240 char 截断）有效防止凭据泄露。不阻断合入，但建议合入前修复以下 2 项：
其余 Medium 项（告警 dedup key 碎片化、 Highlights（正向实践）
评审报告详见: |
3 participants
背景
GitHub issue #123 里 Sentinel 观察到
FLUSH_SEND_ALARM持续约 25 分钟，但现有告警只提供泛化错误文本，缺少可直接定位 SLS 发送失败原因的结构化字段。核心改动
failure_class、status_code、retryable、reason。FLUSH_SEND_ALARM和FLUSH_QUOTA_ALARM保持既有 alarm_type，只追加安全诊断上下文。endpoint_host仅保留 hostname；reason做 URL、AK/SK、token 等去敏并限制长度。修改文件
src/flushers/sls-transport.tssrc/flushers/sls-flusher.tssrc/metrics/alarm-manager.tssrc/metrics/metrics-collector.tsdocs/sls-output.md/docs/zh-CN/sls-output.md/assets/skills/references/sls-diagnostics.mdtests/unit/...验证结果
git diff --check: PASSnpx vitest run tests/unit/flushers/sls-transport.test.ts tests/unit/flushers/sls-flusher.test.ts tests/unit/flushers/sls-flusher.dual-write.test.ts tests/unit/metrics/alarm-manager.test.ts tests/unit/metrics/metrics-collector.test.ts tests/unit/metrics/metrics-writer.test.ts-> 6 files / 107 tests PASSnpm run typecheck: PASSnpm test: 143 files PASS, 1567 tests PASS, 2 skippednpm run build: PASScodex exec触发 3+ STEP / 2+ TOOL，mock SLS 403 / 429 / 500 均命中，failed-log、alarm、metrics 均包含新增字段；429 保留FLUSH_QUOTA_ALARM。docs/trace-validation-rules.json，所以默认node scripts/validate-trace.mjs --latest --format text会因 ENOENT 失败；GitHub Actions PR CI 不调用该脚本，不影响线上 CI。使用脚本支持的显式历史规则文件--rules <(git show aede3d5:docs/trace-validation-rules.json)校验同一条真实 E2E trace 为1 traces, 16 spans, 0 errors, 170 warnings, 0 skipped。准出判断
validate-trace默认规则文件缺失是 base main 已存在的验证工具资料缺失，不属于本 PR 引入，也不会触发 PR CI；本轮作为非阻塞风险记录。