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

Copy spans from memory store, fixes #2719 #2720

Merged
merged 2 commits into from
Jan 9, 2021

Conversation

bobrik
Copy link
Contributor

@bobrik bobrik commented Jan 8, 2021

Copying allows spans to be freely modified by adjusters and any other code without accidentally altering what is stored in the in-memory store itself. Resolves #2719.

@bobrik bobrik requested a review from a team as a code owner January 8, 2021 23:07
@bobrik bobrik requested a review from objectiser January 8, 2021 23:07
@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #2720 (cfb05ac) into master (e788e55) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2720      +/-   ##
==========================================
+ Coverage   95.73%   95.74%   +0.01%     
==========================================
  Files         216      216              
  Lines        9593     9599       +6     
==========================================
+ Hits         9184     9191       +7     
  Misses        336      336              
+ Partials       73       72       -1     
Impacted Files Coverage Δ
plugin/storage/memory/memory.go 100.00% <100.00%> (ø)
plugin/storage/integration/integration.go 77.34% <0.00%> (-0.56%) ⬇️
cmd/query/app/server.go 90.16% <0.00%> (+1.63%) ⬆️

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 e788e55...cfb05ac. Read the comment docs.

plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
Copying allows spans to be freely modified by adjusters and any other code
without accidentally altering what is stored in the in-memory store itself.

Signed-off-by: Ivan Babrou <github@ivan.computer>
yurishkuro
yurishkuro previously approved these changes Jan 9, 2021
@yurishkuro
Copy link
Member

any way to raise the test coverage? It's probably red in the new error handling places.

@bobrik
Copy link
Contributor Author

bobrik commented Jan 9, 2021

Any suggestions on how to approach this? I can't figure out how to trick proto.Marshal to return an error for model.Trace.

@yurishkuro
Copy link
Member

The following results in an error:

	m := &model.Trace{
		Spans: []*model.Span{
			{
				StartTime: time.Date(0, 0, 0, 0, 0, 0, 0, time.UTC),
			},
		},
	}
	_, err := proto.Marshal(m)

Signed-off-by: Ivan Babrou <github@ivan.computer>
@mergify mergify bot dismissed yurishkuro’s stale review January 9, 2021 07:01

Pull request has been modified.

@bobrik
Copy link
Contributor Author

bobrik commented Jan 9, 2021

Added a commit with two more tests to test this scenario, thanks!

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.

Thanks!

@yurishkuro yurishkuro merged commit 2f8ffa9 into jaegertracing:master Jan 9, 2021
bhiravabhatla pushed a commit to bhiravabhatla/jaeger that referenced this pull request Jan 25, 2021
…usters (jaegertracing#2720)

* Copy spans from memory store, fixes jaegertracing#2719

Copying allows spans to be freely modified by adjusters and any other code
without accidentally altering what is stored in the in-memory store itself.

Signed-off-by: Ivan Babrou <github@ivan.computer>

* Add tests to exercise the broken serialization path

Signed-off-by: Ivan Babrou <github@ivan.computer>
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.

Jaeger all-in-one complaints about missing parent spans
2 participants