[ingester/fix] Apply sanitizers to avoid panic on span.process=nil #3819

locmai · 2022-07-20T10:17:54Z

Which problem is this PR solving?

Fix Jaeger ingester panics on convertProcess function #3578

Short description of the changes

Using the process.GetTags() and process.GetServiceName() instead of directly reference to those fields to avoid panic due to nil pointer dereference.

locmai · 2022-07-20T10:21:23Z

plugin/storage/es/spanstore/dbmodel/from_domain_test.go

+	assert.Equal(t, 0, len(dbSpanWithNilTags.Process.Tags))
+	assert.Equal(t, 0, len(dbSpanWithNilProcess.Process.Tags))
+
+	tagsMap := map[string]interface{}(nil)


not sure if this was expected but this is what I got from the convertKeyValuesString func if the Tags was nil.

yurishkuro · 2022-07-20T15:55:45Z

plugin/storage/es/spanstore/dbmodel/from_domain.go

 	return Process{
-		ServiceName: process.ServiceName,
+		ServiceName: process.GetServiceName(),


So this is exactly the type of change that I did not want to make. It avoids a panic, but does it silently, creating invalid data in the process (service name is not allowed to be empty, it could break many other places, including the UI).

I've added a sanitizer in #3631. We can try invoking it early in the Kafka consumer path.

Thanks for taking a look at this and making things clearer. Let me take a look at the sanitizer and try what you suggested.

It make sense that the service name must not be empty, so I might revert the change to process.GetServiceName() here. For the process.GetTags, I think it still nicely handles the nil cases those led to the panics.

I think it still nicely handles the nil cases those led to the panics.

Yes, but again, it does that accidentally, it so happens that in this context the behavior is appropriate. But fundamentally you're trying to work around malformed data that should've never gotten to this point in the pipeline. I don't mind this specific change, but at least with the current code we knew that there is a data issue (even though panicing on it is not ideal reaction).

Hi @yurishkuro , thank you so much for your review. As you mentioned, I understand it's important to detect data issue in our system, so should we return error here and let caller know that we do not accept the nil process for example?

I think panic should be considered if we do want application immediately crash. Otherwise, we can consider error.

Thank you for your explanation and effort.

If we apply the sanitizer earlier in the pipeline then this code does need to change as it will never have to deal with nil process.

yurishkuro · 2022-07-21T15:53:34Z

plugin/storage/es/spanstore/dbmodel/from_domain_test.go

+	assert.Equal(t, 0, len(dbSpanWithNilTags.Process.Tags))
+	assert.Equal(t, 0, len(dbSpanWithNilProcess.Process.Tags))
+
+	tagsMap := map[string]interface{}(nil)


Suggested change

tagsMap := map[string]interface{}(nil)

nilMap := map[string]interface{}(nil)

yurishkuro · 2022-07-21T15:55:45Z

plugin/storage/es/spanstore/dbmodel/from_domain_test.go

+	dbSpanWithNilTags := converter.FromDomainEmbedProcess(&spanWithNilProcessTags)
+	dbSpanWithNilProcess := converter.FromDomainEmbedProcess(&spanWithNilProcess)
+
+	assert.Equal(t, 3, len(dbSpanWithNilTags.Tags))


Suggested change

assert.Equal(t, 3, len(dbSpanWithNilTags.Tags))

assert.Len(t, 3, dbSpanWithNilTags.Tags)

Add `all-in-one` component to the list of components to build docker images for. Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com> Signed-off-by: Loc Mai <locmai0201@gmail.com>

…g#3814) Bumps [github.com/prometheus/common](https://github.com/prometheus/common) from 0.36.0 to 0.37.0. - [Release notes](https://github.com/prometheus/common/releases) - [Commits](prometheus/common@v0.36.0...v0.37.0) --- updated-dependencies: - dependency-name: github.com/prometheus/common dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Loc Mai <locmai0201@gmail.com>

Signed-off-by: Loc Mai <locmai0201@gmail.com>

* Tenancy for queries Signed-off-by: Ed Snible <snible@us.ibm.com> * New parameter for RegisterGRPCGateway() test function Signed-off-by: Ed Snible <snible@us.ibm.com> * More tests that are local to package itself Signed-off-by: Ed Snible <snible@us.ibm.com> * Additional test cases to raise test coverage Signed-off-by: Ed Snible <snible@us.ibm.com> * Fix test Signed-off-by: Ed Snible <snible@us.ibm.com> * spelling Signed-off-by: Ed Snible <snible@us.ibm.com> * Rename file Signed-off-by: Ed Snible <snible@us.ibm.com> * Refactor tenancy packages Signed-off-by: Ed Snible <snible@us.ibm.com> * restore empty_test.go as part of refactoring tenancy out of storage Signed-off-by: Ed Snible <snible@us.ibm.com> * lint/gofumpt Signed-off-by: Ed Snible <snible@us.ibm.com> * Enforce tenancy on non-streaming gRPC and add additional tests Signed-off-by: Ed Snible <snible@us.ibm.com> * Test for tenant flow to storage for both streaming and unary RPC Signed-off-by: Ed Snible <snible@us.ibm.com> * HTTP tenancy test Signed-off-by: Ed Snible <snible@us.ibm.com> * Unit test for unary tenancy handler Signed-off-by: Ed Snible <snible@us.ibm.com> * Factor out rendundent test function Signed-off-by: Ed Snible <snible@us.ibm.com> * Address review comments Signed-off-by: Ed Snible <snible@us.ibm.com> * Error name Signed-off-by: Ed Snible <snible@us.ibm.com> * Refactor TenancyConfig to TenancyManager Signed-off-by: Ed Snible <snible@us.ibm.com> * Address review comments Signed-off-by: Ed Snible <snible@us.ibm.com> * Refactor so that NewTenancyManager() only called from main and tests Signed-off-by: Ed Snible <snible@us.ibm.com> Signed-off-by: Loc Mai <locmai0201@gmail.com>

Signed-off-by: Loc Mai <locmai0201@gmail.com>

… fix/check-nil-process

yurishkuro · 2022-07-25T17:23:27Z

plugin/storage/es/spanstore/dbmodel/from_domain_test.go

+		model.Int64("b.b", 1),
+	}
+
+	spanWithNilTags := model.Span{Tags: tags, Process: &model.Process{Tags: nil}}


This test would pass without any code changes in the PR. It's the nil Process that's causing panic in production, not the nil tags (nil tags are still iterable).

Signed-off-by: Yuri Shkuro <github@ysh.us>

codecov · 2022-07-26T17:07:40Z

Codecov Report

Merging #3819 (9d12426) into main (7b33160) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3819      +/-   ##
==========================================
+ Coverage   97.58%   97.59%   +0.01%     
==========================================
  Files         291      291              
  Lines       16938    16939       +1     
==========================================
+ Hits        16529    16532       +3     
+ Misses        323      321       -2     
  Partials       86       86

Impacted Files	Coverage Δ
cmd/ingester/app/processor/span_processor.go	`100.00% <100.00%> (ø)`
pkg/config/tlscfg/cert_watcher.go	`94.68% <0.00%> (+2.12%)`	⬆️

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 7b33160...9d12426. Read the comment docs.

…aegertracing#3819)

locmai requested a review from a team as a code owner July 20, 2022 10:17

locmai requested a review from vprithvi July 20, 2022 10:17

locmai commented Jul 20, 2022

View reviewed changes

locmai force-pushed the fix/check-nil-process branch 2 times, most recently from fb2a295 to d21e22d Compare July 20, 2022 10:26

yurishkuro reviewed Jul 20, 2022

View reviewed changes

yurishkuro reviewed Jul 21, 2022

View reviewed changes

locmai force-pushed the fix/check-nil-process branch from dd9064b to 7b08a60 Compare July 25, 2022 08:32

albertteoh and others added 9 commits July 25, 2022 15:35

Add capability to build docker all-in-one (jaegertracing#3813)

8fcb586

Add `all-in-one` component to the list of components to build docker images for. Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com> Signed-off-by: Loc Mai <locmai0201@gmail.com>

fix: check nil process to avoid nil pointer

d761686

Signed-off-by: Loc Mai <locmai0201@gmail.com>

fix: use GetTags instead

817cdf1

Signed-off-by: Loc Mai <locmai0201@gmail.com>

chore: add test cases

c8a8458

Signed-off-by: Loc Mai <locmai0201@gmail.com>

chore: add sanitizer on span processor

385f792

Signed-off-by: Loc Mai <locmai0201@gmail.com>

fix: check nil process to avoid nil pointer

bbcb3df

Signed-off-by: Loc Mai <locmai0201@gmail.com>

Tenancy for memory storage (jaegertracing#3827)

26a4398

Signed-off-by: Loc Mai <locmai0201@gmail.com>

locmai force-pushed the fix/check-nil-process branch from 7b08a60 to 26a4398 Compare July 25, 2022 08:36

locmai added 3 commits July 25, 2022 15:36

Merge branch 'main' into fix/check-nil-process

265e15f

chore: add sanitizer on span processor

94ca83f

Signed-off-by: Loc Mai <locmai0201@gmail.com>

Merge branch 'fix/check-nil-process' of github.com:locmai/jaeger into…

248184f

… fix/check-nil-process

yurishkuro reviewed Jul 25, 2022

View reviewed changes

yurishkuro mentioned this pull request Jul 26, 2022

[Bug]: jaeger ingester module nil pointer error (when consume to ES) #3829

Open

Simplify the fix

9d12426

Signed-off-by: Yuri Shkuro <github@ysh.us>

yurishkuro approved these changes Jul 26, 2022

View reviewed changes

yurishkuro changed the title ~~fix: check nil process~~ [ingester/fix] Apply sanitizers to avoid panic on span.process=nil Jul 26, 2022

yurishkuro merged commit a5974e1 into jaegertracing:main Jul 26, 2022

gburton1 mentioned this pull request Aug 8, 2022

Upgrade Jaeger appVersion for the chart jaegertracing/helm-charts#391

Merged

1 task

albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Aug 8, 2022

[ingester/fix] Apply sanitizers to avoid panic on span.process=nil (j…

db7f5a1

…aegertracing#3819)

juan-ramirez-sp mentioned this pull request Aug 16, 2022

Jaeger ingester panics on convertProcess function #3578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ingester/fix] Apply sanitizers to avoid panic on span.process=nil #3819

[ingester/fix] Apply sanitizers to avoid panic on span.process=nil #3819

locmai commented Jul 20, 2022

locmai Jul 20, 2022

yurishkuro Jul 20, 2022

locmai Jul 21, 2022

yurishkuro Jul 21, 2022

trangmaiq Jul 22, 2022

yurishkuro Jul 22, 2022

yurishkuro Jul 21, 2022

yurishkuro Jul 21, 2022

yurishkuro Jul 25, 2022

codecov bot commented Jul 26, 2022

	tagsMap := map[string]interface{}(nil)
	nilMap := map[string]interface{}(nil)

	assert.Equal(t, 3, len(dbSpanWithNilTags.Tags))
	assert.Len(t, 3, dbSpanWithNilTags.Tags)

[ingester/fix] Apply sanitizers to avoid panic on span.process=nil #3819

[ingester/fix] Apply sanitizers to avoid panic on span.process=nil #3819

Conversation

locmai commented Jul 20, 2022

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2022

Codecov Report