Skip to content

Fix SpanToJaegerMapper for short trace id handling#135

Merged
sergeyklay merged 4 commits intojonahgeorge:masterfrom
croissant-sama:fix-jaeger-mapper-for-short-trace-id
Aug 19, 2025
Merged

Fix SpanToJaegerMapper for short trace id handling#135
sergeyklay merged 4 commits intojonahgeorge:masterfrom
croissant-sama:fix-jaeger-mapper-for-short-trace-id

Conversation

@croissant-sama
Copy link
Copy Markdown
Contributor

Some systems remove leading zeros from trace IDs. This PR adds support for short trace IDs without leading zeros.
Implemented according to the approach of the Golang library uber/jaeger-client-go.

@croissant-sama croissant-sama marked this pull request as draft July 16, 2025 07:34
@croissant-sama croissant-sama marked this pull request as ready for review July 16, 2025 07:35
@croissant-sama croissant-sama marked this pull request as draft July 16, 2025 07:37
@croissant-sama croissant-sama marked this pull request as ready for review July 16, 2025 07:39
@croissant-sama
Copy link
Copy Markdown
Contributor Author

@sergeyklay
Hi! I've made a small fix. Could you please review my changes?

@sergeyklay
Copy link
Copy Markdown
Collaborator

Hey @croissant-sama ! Thanks a lot for your PR and for taking the time to contribute.

I just wanted to let you know that the current changes are causing a syntax error in the tests, so the CI is failing on all PHP versions. The error message is:

ParseError: syntax error, unexpected ';', expecting variable (T_VARIABLE) in tests/Jaeger/Mapper/SpanToJaegerMapperTest.php:184

It looks like there's a typo in the test file you updated (line 184). Could you double-check that section and fix the syntax error? Once that’s fixed, the CI should pass and I’ll be happy to review your changes again.

NB: I also noticed the workflow was failing because it was using a deprecated version of actions/cache@v2. I went ahead and updated it to v4 in your branch to fix this problem, so you don’t need to worry about that one.

Let me know if you have any questions

Copy link
Copy Markdown
Collaborator

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

The changes currently cause a syntax error in the tests

@croissant-sama
Copy link
Copy Markdown
Contributor Author

@sergeyklay I made the necessary fix.

@croissant-sama
Copy link
Copy Markdown
Contributor Author

Hey @sergeyklay! Could you please review my changes?

@sergeyklay sergeyklay merged commit f98ce29 into jonahgeorge:master Aug 19, 2025
6 checks passed
@sergeyklay
Copy link
Copy Markdown
Collaborator

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