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

feature: add streamingSpanWriterPlugin #3640

Merged
merged 3 commits into from
May 4, 2022
Merged

Conversation

vuuihc
Copy link
Contributor

@vuuihc vuuihc commented Apr 24, 2022

Signed-off-by: vuuihc [email protected]

Which problem is this PR solving?

Resolves #3636

Short description of the changes

add streamingSpanWriterPlugin to improve the write performance of grpc plugin.

@vuuihc vuuihc requested a review from a team as a code owner April 24, 2022 17:31
@vuuihc vuuihc requested a review from pavolloffay April 24, 2022 17:31
@codecov
Copy link

codecov bot commented Apr 24, 2022

Codecov Report

Merging #3640 (cbfb854) into main (784644c) will increase coverage by 0.10%.
The diff coverage is 96.05%.

@@            Coverage Diff             @@
##             main    #3640      +/-   ##
==========================================
+ Coverage   96.40%   96.50%   +0.10%     
==========================================
  Files         264      265       +1     
  Lines       15509    15581      +72     
==========================================
+ Hits        14951    15037      +86     
+ Misses        468      455      -13     
+ Partials       90       89       -1     
Impacted Files Coverage Δ
plugin/storage/grpc/grpc.go 0.00% <0.00%> (ø)
plugin/storage/grpc/shared/plugin.go 0.00% <0.00%> (ø)
plugin/storage/grpc/factory.go 95.08% <100.00%> (+0.43%) ⬆️
plugin/storage/grpc/memory/plugin.go 100.00% <100.00%> (ø)
plugin/storage/grpc/shared/grpc_client.go 85.79% <100.00%> (+6.72%) ⬆️
plugin/storage/grpc/shared/grpc_server.go 76.15% <100.00%> (+1.87%) ⬆️
plugin/storage/grpc/shared/streaming_writer.go 100.00% <100.00%> (ø)
cmd/query/app/static_handler.go 97.60% <0.00%> (+0.59%) ⬆️
cmd/collector/app/server/zipkin.go 70.73% <0.00%> (+12.19%) ⬆️

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 784644c...cbfb854. Read the comment docs.

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 for the PR.

I feel like I am missing something. It's not clear to me why we need a separate "streaming client" writer, could we not do it from the existing client?

But more importantly, I think the overall approach we had taken when we extended the plugin contract with "archive" storage was to add Capabilities field that tells the client if the plugin implementation supports additional interfaces. So I am surprised that you don't have any changed to the Factory files.

Lastly, I would like to have integration tests to exercise the new functionality.

plugin/storage/grpc/shared/streaming_writer_grpc_client.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/streaming_writer_grpc_client.go Outdated Show resolved Hide resolved
plugin/storage/grpc/config/config.go Outdated Show resolved Hide resolved
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.

lgtm overall, I kicked off the tests

plugin/storage/grpc/factory.go Show resolved Hide resolved
plugin/storage/grpc/shared/streaming_writer.go Outdated Show resolved Hide resolved
examples/memstore-plugin/main.go Outdated Show resolved Hide resolved
@vuuihc
Copy link
Contributor Author

vuuihc commented Apr 27, 2022

@yurishkuro Hi yuri, all the issues you commented have been resolved.

@vuuihc vuuihc requested a review from yurishkuro April 29, 2022 04:13
@vuuihc vuuihc requested a review from yurishkuro April 30, 2022 15:51
@vuuihc vuuihc requested a review from yurishkuro May 2, 2022 04:09
@yurishkuro
Copy link
Member

I will try to do a final review tomorrow

@yurishkuro yurishkuro merged commit 99ec00f into jaegertracing:main May 4, 2022
yurishkuro added a commit to yurishkuro/jaeger that referenced this pull request May 4, 2022
yurishkuro added a commit that referenced this pull request May 4, 2022
albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Jul 13, 2022
albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Jul 13, 2022
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.

Add a client stream WriteSpan method in grpc storage plugin
2 participants