Adds encoders.#8
Conversation
jcchavezs
force-pushed
the
adds_encoders
branch
2 times, most recently
from
9024a0f to
9a48880
Compare
kevinlebrun
left a comment
kevinlebrun
left a comment
There was a problem hiding this comment.
There are weird things going on around those micro-seconds time management. Are strings an option?
| * @param Span $span | ||
| * @return array | ||
| */ | ||
| private static function spanToArray(Span $span) |
There was a problem hiding this comment.
Why is this method declared static? Is an instance method not good enough?
There was a problem hiding this comment.
Good question, don't remember the answer but will check it out later.
| return $arraySpan; | ||
| } | ||
|
|
||
| private static function encodeSpan(Span $span) |
There was a problem hiding this comment.
Same as for spanToArray.
| } | ||
|
|
||
| if ($span->isFinished()) { | ||
| $arraySpan['duration_micro'] = 0; |
There was a problem hiding this comment.
It seems a little hacky. Why are you doing the replace inside encodeSpan? If it is a matter of int precision even with those rare 32-bit runtime you have more then 2000second to store the duration.
There was a problem hiding this comment.
It was a matter of consistency. Since both start and duration are being stored as micro and then converted into nano when encoding, doing so only for start and not for duration seemed weird to me. If this bugs you too much we can have a chat and get into an agreement. WDYT?
| 'name' => $span->getOperationName(), | ||
| 'resource' => $span->getResource(), | ||
| 'service' => $span->getService(), | ||
| 'start_micro' => 0, |
There was a problem hiding this comment.
It seems a little hacky.
| { | ||
| $expectedPayload = <<<JSON | ||
| [[{"trace_id":"160e7072ff7bd5f1","span_id":"160e7072ff7bd5f2","name":"test_name","resource":"test_resource", | ||
| JSON |
There was a problem hiding this comment.
Why is this concatenation necessary?
There was a problem hiding this comment.
This was because maximum line length per file (linters). The way to avoid this is to ident the json fields but that screw up the comparison in line 31. Remember, we can't convert the json into array/object in order to compare it because of the lack of support for int64 in PHP.
|
Not sure about the string @kevinlebrun. @palazzem is it possible to send the |
jcchavezs
force-pushed
the
adds_encoders
branch
from
9a48880 to
1e88111
Compare
|
@jcchavezs actually the Trace Agent (written in Go) expects an This happens for the MsgPack Encoder, while for the JSON Encoder you'll get: |
|
I guess since this is the only way to pass the int64 we can merge this PR. What do you think @kevinlebrun ? |
|
Yes, totally. |
|
@kevinlebrun @palazzem can you approve this PR? |
3 participants
This depends on #7
Ping @palazzem @vlad-mh @kevinlebrun