Add unmarshalling for pdata.Traces #1948
Conversation
pkositsyn
requested review from
bogdandrutu,
dmitryax,
james-bebbington,
nilebox,
owais,
pjanotti and
tigrannajaryan
as code owners
|
|
Codecov Report
@@ Coverage Diff @@
## master #1948 +/- ##
=======================================
Coverage 91.41% 91.41%
=======================================
Files 284 284
Lines 16788 16793 +5
=======================================
+ Hits 15347 15352 +5
Misses 1009 1009
Partials 432 432
Continue to review full report at Codecov.
|
pkositsyn
force-pushed
the
unmarshall_pdata_traces
branch
from
d509d24
to
925a9ef
Compare
| if err := proto.Unmarshal(data, traces); err != nil { | ||
| return err | ||
| } | ||
| *td.orig = traces.ResourceSpans |
There was a problem hiding this comment.
This won't work for zero-initialized td. Instead I suggest to change FromOtlpProtoBytes to take a pointer receiver and here do
td.orig = &traces.ResourceSpansThere was a problem hiding this comment.
If you look at similar ToOtlpProtoBytes - it has the same problem (actually I see no problem, no one guarantees zero-initialized structure to work properly). I cannot change abovementioned function, because this would break compatibility, and in my opinion it's better to be consistent in this case. Or even return to the first solution where function wasn't on the structure
There was a problem hiding this comment.
Hmm, it is unfortunate that we used value receivers on Traces, but it is too late now, so you are right, we need to use value receivers for consistency now. Please add a comment to FromOtlpProtoBytes to tell that NewTraces must be used to create the instance before calling this function. It is easy to make the mistake since one will probably try this first and will fail:
var td pdata.Traces
td.FromOtlpProtoBytes(someData)
|
@tigrannajaryan Added comment. Should be good to go |
|
Thanks @Vemmy124 |
Description:
Adding a feature - allow to unmarshal pdata.Traces from protobuf
Link to tracking Issue: #1939
Testing: unmarshal(marshal(trace)) = trace