Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix null pointer in jaeger-spark-dependencies #2327

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

moolen
Copy link
Contributor

@moolen moolen commented Jul 4, 2020

Which problem is this PR solving?

fixes jaegertracing/jaeger-operator#1113

I'm not sure if other fields have the same issue.

@moolen moolen requested a review from a team as a code owner July 4, 2020 16:28
@moolen moolen requested a review from jpkrohling July 4, 2020 16:28
@yurishkuro
Copy link
Member

Is the expectation that any list must not be null, or just references? For example, logs() function also may return nil.

@moolen
Copy link
Contributor Author

moolen commented Jul 4, 2020

It depends on the consumer, i guess. speaking for jaeger-spark-dependencies: tags and process must be non-nil (otherwise we'll see a null pointer exception).

see SpanDeserializer.java / ObjectMapper.java.

While taking a look at #2295:

OTEL data model is directly transformed to Jaeger ES model

I think writing null into $storage is a bug.

@pavolloffay you're much deeper into this, can you confirm this? If so i'll change tags and process accordingly.

*edit* i fixed tags and process aswell

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link

codecov bot commented Jul 4, 2020

Codecov Report

Merging #2327 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...lugin/sampling/strategystore/adaptive/processor.go 99.20% <0.00%> (-0.80%) ⬇️
plugin/storage/integration/integration.go 80.38% <0.00%> (-0.48%) ⬇️
cmd/query/app/server.go 93.18% <0.00%> (+2.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5001225...da7ec4e. Read the comment docs.

@moolen moolen force-pushed the fix/empty-ref branch 2 times, most recently from 969aa0e to e1937c6 Compare July 6, 2020 05:35
@@ -131,10 +131,12 @@ func toTime(nano pdata.TimestampUnixNano) time.Time {
return time.Unix(0, int64(nano)).UTC()
}

var emptyReferenceList = []dbmodel.Reference{}
Copy link
Member

Choose a reason for hiding this comment

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

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>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thank you!

@yurishkuro yurishkuro merged commit 97d2319 into jaegertracing:master Jul 7, 2020
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.

Test failing -- TestElasticSearchSuite/TestSparkDependenciesES
2 participants