fix null pointer in jaeger-spark-dependencies #2327
Conversation
|
Is the expectation that any list must not be null, or just references? For example, |
|
It depends on the consumer, i guess. speaking for see SpanDeserializer.java / ObjectMapper.java. While taking a look at #2295:
I think writing @pavolloffay you're much deeper into this, can you confirm this? If so i'll change *edit* i fixed |
| @@ -134,7 +134,7 @@ func toTime(nano pdata.TimestampUnixNano) time.Time { | |||
| func references(links pdata.SpanLinkSlice, parentSpanID pdata.SpanID, traceID dbmodel.TraceID) ([]dbmodel.Reference, error) { | |||
| parentSpanIDSet := len(parentSpanID.Bytes()) != 0 | |||
| if !parentSpanIDSet && links.Len() == 0 { | |||
| return nil, nil | |||
| return []dbmodel.Reference{}, nil | |||
There was a problem hiding this comment.
we could avoid some memory allocations by using constants in these cases, like
const emptyReferenceList = []dbmodel.Reference{}
the most impact will be for logs, since it's quite rare to find spans without references or tags.
BTW, I wonder if there is another bug in the translator from OTEL that fails to create a CHILD_OF reference to the parent span ID, which is required in the Jaeger model since we don't store parentSpanID in the database.
There was a problem hiding this comment.
Thanks for your feedback, i made changes accordingly.
I don't see where there could be a bug (...or i don't get it 😄 ). The CHILD_OF refs are tested properly as far as i can tell. It either creates a reference or it errors out.
Codecov Report
@@ Coverage Diff @@
## master #2327 +/- ##
==========================================
- Coverage 95.65% 95.64% -0.01%
==========================================
Files 205 205
Lines 10529 10529
==========================================
- Hits 10071 10070 -1
- Misses 391 393 +2
+ Partials 67 66 -1
Continue to review full report at Codecov.
|
moolen
force-pushed
the
fix/empty-ref
branch
2 times, most recently
from
969aa0e
to
e1937c6
Compare
| @@ -131,10 +131,12 @@ func toTime(nano pdata.TimestampUnixNano) time.Time { | |||
| return time.Unix(0, int64(nano)).UTC() | |||
| } | |||
|
|
|||
| var emptyReferenceList = []dbmodel.Reference{} | |||
There was a problem hiding this comment.
please group all vars at the top of the file (after constants, if any)
var (
emptyReferenceList = []dbmodel.Reference{}
emptyTagList = []dbmodel.KeyValue{}
etc
)
fixes jaegertracing/jaeger-operator#1113 Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Which problem is this PR solving?
fixes jaegertracing/jaeger-operator#1113
I'm not sure if other fields have the same issue.