fix(zipkin): store remote endpoint IPs as text, not raw binary/int#1998
Conversation
Zipkin's V2 JSON API expects ipv4/ipv6 on Endpoint as textual strings. Storing raw inet_pton() bytes for IPv6 produced invalid UTF-8, crashing json_encode(..., JSON_THROW_ON_ERROR) and dropping the whole span batch for any CLIENT/PRODUCER span with a net.peer.ip IPv6 address. ipv4 had the same class of bug, encoding as an integer instead of a dotted string.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1998 +/- ##
============================================
+ Coverage 68.02% 68.08% +0.05%
- Complexity 3075 3082 +7
============================================
Files 459 459
Lines 9017 9026 +9
============================================
+ Hits 6134 6145 +11
+ Misses 2883 2881 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes Zipkin V2 JSON endpoint IP serialization by ensuring remoteEndpoint.ipv4/remoteEndpoint.ipv6 are stored as textual IP strings (preventing invalid UTF-8 / incorrect numeric encodings) and adds regression coverage to avoid span-batch drops during JSON encoding.
Changes:
- Store
remoteEndpoint.ipv4as a dotted IPv4 string instead ofip2long()output. - Store
remoteEndpoint.ipv6as a textual IPv6 string (instead of rawinet_pton()bytes) with regression coverage for routable IPv6 +json_encode(..., JSON_THROW_ON_ERROR). - Refactor Zipkin remote endpoint tests into a data provider covering IPv4/IPv6 cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Contrib/Zipkin/SpanConverter.php | Adjusts remote endpoint IP fields to be serialized as text for Zipkin V2 JSON. |
| tests/Unit/Contrib/Zipkin/ZipkinSpanConverterTest.php | Adds/updates unit coverage for IPv4/IPv6 remote endpoint serialization and JSON encoding regression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string $expectedKey, | ||
| string $expectedValue, | ||
| int $expectedPort, | ||
| ): void { |
There was a problem hiding this comment.
Not relevant. This code is correct according to the CI's style Check (PHP CS Fixer).
|
@jorgsowa - ready to merge this after the comments are addressed. Thanks! |
ce211ec to
cd7c5f6
Compare
|
I applied the first comment, but not the second one. Style Check is passing, and PHP CS Fixer prevents the suggested form. |
Both inet_pton() and inet_ntop() can return false. Check inet_pton() result before passing to inet_ntop(), and check inet_ntop() result before assigning to remoteEndpoint['ipv6']. Fall back to the original IP string if either conversion fails.
cd7c5f6 to
b4faedd
Compare
Zipkin's V2 JSON API expects ipv4/ipv6 on Endpoint as textual strings. Storing raw inet_pton() bytes for IPv6 produced invalid UTF-8, crashing json_encode(..., JSON_THROW_ON_ERROR) and dropping the whole span batch for any CLIENT/PRODUCER span with a net.peer.ip IPv6 address. ipv4 had the same class of bug, encoding as an integer instead of a dotted string.