Skip to content

Add typechecking to the Jaeger propagator - #5440

Open
Eason09053360 wants to merge 9 commits into
open-telemetry:mainfrom
Eason09053360:fix-jaeger-parse-trace-id-header-annotation
Open

Add typechecking to the Jaeger propagator#5440
Eason09053360 wants to merge 9 commits into
open-telemetry:mainfrom
Eason09053360:fix-jaeger-parse-trace-id-header-annotation

Conversation

@Eason09053360

@Eason09053360 Eason09053360 commented Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

_parse_trace_id_header in the Jaeger propagator is annotated as returning tuple[int] (a tuple of exactly one element), but every return path yields a 3-tuple (trace_id, span_id, flags) and the caller unpacks three values. This misleads type checkers.

This PR fixes the annotation to tuple[int, int, int]. Annotation-only change, no runtime impact.
_extract_baggage — Add complete type annotation

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Annotation-only change; existing propagator/opentelemetry-propagator-jaeger tests cover the extraction behavior.

  • tox -e typecheck

typecheck: commands[0]> pyright --version
WARNING: there is a new pyright version available (v1.1.405 -> v1.1.411).
Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest`

pyright 1.1.405
typecheck: commands[1]> pyright
WARNING: there is a new pyright version available (v1.1.405 -> v1.1.411).
Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest`

0 errors, 0 warnings, 0 informations
  typecheck: OK (2.88=setup[0.01]+cmd[0.52,2.35] seconds)
  congratulations :) (2.99 seconds)

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated

Eason09053360 added a commit to Eason09053360/opentelemetry-python that referenced this pull request Jul 19, 2026
@Eason09053360
Add changelog fragment for open-telemetry#5440
75f22c1
@Eason09053360
Eason09053360 marked this pull request as ready for review July 19, 2026 13:57
@Eason09053360
Eason09053360 requested a review from a team as a code owner July 19, 2026 13:57
@Eason09053360
Fix return type annotation of _parse_trace_id_header in Jaeger propag…
3ccc2ab 
…ator

The function returns a 3-tuple (trace_id, span_id, flags) on every code
path and callers unpack three values, but the annotation declared
tuple[int] (a one-element tuple), which misleads type checkers.
@Eason09053360
Add changelog fragment for open-telemetry#5440
9fcea30
@Eason09053360
Eason09053360 force-pushed the fix-jaeger-parse-trace-id-header-annotation branch from 75f22c1 to 9fcea30 Compare July 24, 2026 01:00
@xrmx

xrmx commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Change looks good, any chance you can fix all the typing issue in the package so we can enable typechecking? Your PR already fixes half of them. Add the following and then uv run tox -e typecheck

diff --git a/pyproject.toml b/pyproject.toml
index 51bc40983..302fc593c 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -133,7 +133,8 @@ include = [
   "exporter/opentelemetry-exporter-otlp-proto-http",
   "exporter/opentelemetry-exporter-otlp-json-common",
   "exporter/opentelemetry-exporter-otlp-common",
-  "codegen/opentelemetry-codegen-json"
+  "codegen/opentelemetry-codegen-json",
+  "propagator/opentelemetry-propagator-jaeger",
 ]
 
 exclude = [
@@ -154,6 +155,7 @@ exclude = [
   "exporter/opentelemetry-exporter-otlp-json-common/tests",
   "exporter/opentelemetry-exporter-otlp-common/tests",
   "exporter/opentelemetry-exporter-otlp-json-common/benchmarks",
+  "propagator/opentelemetry-propagator-jaeger/tests",
 ]
 
 # When packages are correct typed add them to the strict list

@xrmx xrmx moved this to Easy to review / merge / close in Python PR digest Jul 24, 2026
@Eason09053360
Fix remaining typing issues in Jaeger propagator and enable typechecking
7343087 
Parameterize Getter/Setter with CarrierT to match the base class
signatures, guard against None baggage values before unquoting, type
header values as str instead of the unbound CarrierT, and make
_int_from_hex_str return int | None with explicit fallbacks at the call
sites. Add the package to the pyright include list.
@Eason09053360

Eason09053360 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Change looks good, any chance you can fix all the typing issue in the package so we can enable typechecking? Your PR already fixes half of them. Add the following and then uv run tox -e typecheck

diff --git a/pyproject.toml b/pyproject.toml
index 51bc40983..302fc593c 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -133,7 +133,8 @@ include = [
   "exporter/opentelemetry-exporter-otlp-proto-http",
   "exporter/opentelemetry-exporter-otlp-json-common",
   "exporter/opentelemetry-exporter-otlp-common",
-  "codegen/opentelemetry-codegen-json"
+  "codegen/opentelemetry-codegen-json",
+  "propagator/opentelemetry-propagator-jaeger",
 ]
 
 exclude = [
@@ -154,6 +155,7 @@ exclude = [
   "exporter/opentelemetry-exporter-otlp-json-common/tests",
   "exporter/opentelemetry-exporter-otlp-common/tests",
   "exporter/opentelemetry-exporter-otlp-json-common/benchmarks",
+  "propagator/opentelemetry-propagator-jaeger/tests",
 ]
 
 # When packages are correct typed add them to the strict list

Thanks for the review! Sure, I can do that. I've updated pyproject.toml to include the jaeger propagator, fixed the remaining typing issues, and made sure that uv run tox -e typecheck passes successfully ：）

@github-project-automation github-project-automation Bot moved this from Easy to review / merge / close to Approved PRs in Python PR digest Aug 3, 2026
@ocelotl

ocelotl commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

please fix conflicts

@xrmx xrmx changed the title Fix return type annotation of _parse_trace_id_header in Jaeger propagator Add typechecking to the Jaeger propagator Aug 12, 2026
aabmass
aabmass approved these changes Aug 13, 2026
View reviewed changes

@aabmass aabmass left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one nit but LGTM

Comment thread propagator/opentelemetry-propagator-jaeger/src/opentelemetry/propagators/jaeger/__init__.py Outdated
@aabmass aabmass moved this from Approved PRs to Approved PRs that need fixes in Python PR digest Aug 13, 2026
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-08-15 12:58 UTC

Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):

  • Top-level threads: 1
Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
  • Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

@opentelemetry-pr-dashboard opentelemetry-pr-dashboard Bot mentioned this pull request Aug 14, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xrmx xrmx xrmx approved these changes
@aabmass aabmass aabmass approved these changes
@lzchen lzchen lzchen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Approved PRs that need fixes

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Eason09053360 @xrmx @ocelotl @aabmass @lzchen