Skip to content

Conversation

@dengyh
Copy link
Collaborator

@dengyh dengyh commented Jan 22, 2026

No description provided.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

代码审查总结

审查了跨服务追踪功能的实现,发现 4 个需要关注的问题:

🚨 1 个严重问题: - 全局字典在多进程环境下存在内存泄露风险
⚠️ 2 个逻辑/性能问题:

    • setup_propagators 时机可能过早
    • 重复注入 trace headers
      1 个改进建议: - 私有属性访问兼容性

详细问题

🚨 celery_trace.py:35 - 全局字典泄露风险

_task_context_tokens 是模块级全局字典,在多进程 Celery worker 环境下可能导致 task_id 冲突或内存泄露。如果 task_postrun 信号未触发(如 worker 异常退出),token 将永久残留。

建议: 使用 Celery 的 context locals 或为每个 worker 进程独立存储。

⚠️ apps.py:43 - setup_propagators 时机风险

apps.py:ready() 中调用 setup_propagators(),但此时 OpenTelemetry 可能尚未初始化(返回 ProxyTracerProvider),导致 BaggageToSpanProcessor 注册失败。

建议: 在 OpenTelemetry 初始化完成后再调用,或添加重试机制。

⚠️ http.py:85-88 - 重复注入 headers

_http_request() 中,即使调用方已通过 _gen_header() 注入过 trace headers,仍会再次调用 inject_trace_headers(headers),造成不必要的重复操作。

建议: 检查 headers 中是否已有 traceparent 再决定是否注入。

✨ trace.py:77 - 异常处理增强

setup_propagators() 访问 provider._active_span_processor._span_processors 使用了私有属性,在不同 OpenTelemetry 版本中可能不兼容。

建议: 使用 try-except 包裹或检查属性是否存在。

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

代码审查总结

审查了跨服务追踪功能的实现,发现 4 个需要关注的问题:

🚨 1 个严重问题: celery_trace.py:35 - 全局字典在多进程环境下存在内存泄露风险
⚠️ 2 个逻辑/性能问题:

  • apps.py:43 - setup_propagators 时机可能过早
  • http.py:88 - 重复注入 trace headers
    1 个改进建议: trace.py:77 - 私有属性访问兼容性

详细问题

🚨 celery_trace.py:35 - 全局字典泄露风险

_task_context_tokens 是模块级全局字典,在多进程 Celery worker 环境下可能导致 task_id 冲突或内存泄露。如果 task_postrun 信号未触发(如 worker 异常退出),token 将永久残留。

建议: 使用 Celery 的 context locals 或为每个 worker 进程独立存储。

⚠️ apps.py:43 - setup_propagators 时机风险

apps.py:ready() 中调用 setup_propagators(),但此时 OpenTelemetry 可能尚未初始化(返回 ProxyTracerProvider),导致 BaggageToSpanProcessor 注册失败。

建议: 在 OpenTelemetry 初始化完成后再调用,或添加重试机制。

⚠️ http.py:85-88 - 重复注入 headers

_http_request() 中,即使调用方已通过 _gen_header() 注入过 trace headers,仍会再次调用 inject_trace_headers(headers),造成不必要的重复操作。

建议: 检查 headers 中是否已有 traceparent 再决定是否注入。

✨ trace.py:77 - 异常处理增强

setup_propagators() 访问 provider._active_span_processor._span_processors 使用了私有属性,在不同 OpenTelemetry 版本中可能不兼容。

建议: 使用 try-except 包裹或检查属性是否存在。

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 72.78912% with 40 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@011cdfe). Learn more about missing BASE report.

Files with missing lines Patch % Lines
bkflow/utils/trace.py 78.09% 23 Missing ⚠️
bkflow/utils/celery_trace.py 46.87% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #591   +/-   ##
=========================================
  Coverage          ?   79.94%           
=========================================
  Files             ?      240           
  Lines             ?    13301           
  Branches          ?        0           
=========================================
  Hits              ?    10634           
  Misses            ?     2667           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

2 participants