Update storage api to support query operations by spanKind #1942

guo0693 · 2019-11-22T19:46:40Z

Which problem is this PR solving?

Relates to Cassandra & memory storage update: query service to support spanKind when retrieve operations for a given service. #1921

Short description of the changes

storage API changes to support new query parameter and return type (across all storage engines)

This PR contains the exact same changes as guo0693#1, The only difference is this PR is opened against master branch.

…n for operation name index - add migration script - read from the latest table if available, otherwise fail back to previous table Signed-off-by: Jun Guo <guo0693@gmail.com>

Signed-off-by: Jun Guo <guo0693@gmail.com>

…aegertracing#1921-2

Signed-off-by: Jun Guo <guo0693@gmail.com>

# Conflicts: # plugin/storage/cassandra/spanstore/operation_names.go # plugin/storage/cassandra/spanstore/operation_names_test.go # plugin/storage/cassandra/spanstore/reader_test.go # plugin/storage/cassandra/spanstore/writer_test.go

Signed-off-by: Jun Guo <guo0693@gmail.com>

yurishkuro

in many places SpanKind: "" is redundant

cmd/query/app/querysvc/query_service.go

plugin/storage/badger/spanstore/cache.go

plugin/storage/badger/spanstore/reader.go

plugin/storage/cassandra/spanstore/operation_names.go

plugin/storage/grpc/shared/grpc_server.go

plugin/storage/grpc/shared/grpc_server_test.go

plugin/storage/integration/kafka_test.go

storage/spanstore/metrics/decorator.go

codecov · 2019-11-22T20:17:04Z

Codecov Report

Merging #1942 into master will decrease coverage by 0.01%.
The diff coverage is 98.14%.

@@            Coverage Diff             @@
##           master    #1942      +/-   ##
==========================================
- Coverage   98.44%   98.43%   -0.02%     
==========================================
  Files         198      198              
  Lines        9794     9847      +53     
==========================================
+ Hits         9642     9693      +51     
- Misses        115      116       +1     
- Partials       37       38       +1

Impacted Files	Coverage Δ
plugin/storage/memory/memory.go	`100% <100%> (ø)`	⬆️
plugin/storage/cassandra/spanstore/reader.go	`100% <100%> (ø)`	⬆️
plugin/storage/badger/spanstore/reader.go	`96.75% <100%> (ø)`	⬆️
plugin/storage/badger/spanstore/cache.go	`100% <100%> (ø)`	⬆️
storage/spanstore/metrics/decorator.go	`100% <100%> (ø)`	⬆️
plugin/storage/cassandra/spanstore/writer.go	`100% <100%> (ø)`	⬆️
...gin/storage/cassandra/spanstore/operation_names.go	`97.59% <100%> (ø)`	⬆️
plugin/storage/es/spanstore/reader.go	`100% <100%> (ø)`	⬆️
plugin/storage/grpc/shared/grpc_client.go	`85.96% <100%> (+1.8%)`	⬆️
plugin/storage/grpc/shared/grpc_server.go	`76.47% <100%> (+2.44%)`	⬆️
... and 1 more

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 11cf2b9...81ca941. Read the comment docs.

- shorten var names and wrap lines - make grpc server backward compatible - write struct instead of list of strings for operation_names Signed-off-by: Jun Guo <guo0693@gmail.com>

plugin/storage/cassandra/spanstore/operation_names.go

plugin/storage/badger/spanstore/reader.go

plugin/storage/cassandra/spanstore/dbmodel/operation.go

plugin/storage/cassandra/spanstore/operation_names.go

plugin/storage/grpc/shared/grpc_server.go

plugin/storage/integration/kafka_test.go

plugin/storage/memory/memory_test.go

- Backward compatibility for grpc client Signed-off-by: Jun Guo <guo0693@gmail.com>

yurishkuro · 2019-11-24T03:22:31Z

But we need to merge the PRs in reverse order into chained branches. E.g. PR3 merge to branch 2 then PR2 merge into branch 1, then PR1 into master.

I've never tried this, but GH allows you to change the base branch of a PR: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-base-branch-of-a-pull-request

guo0693 · 2019-11-25T15:57:01Z

But we need to merge the PRs in reverse order into chained branches. E.g. PR3 merge to branch 2 then PR2 merge into branch 1, then PR1 into master.

I've never tried this, but GH allows you to change the base branch of a PR: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-base-branch-of-a-pull-request

It doesn't work that well for us:
master
branch1
branch2

assume branch1 has several commits, branch2 is based on branch1. After branch1 merged to master, those commits are squashed, when we update PR from branch2 to compare to master, the commit history is different, we will need to merge master into branch2 and solve merge conflicts first, this will apply to all the dependent branches and require extra reviews.

guo0693 · 2019-11-25T15:57:41Z

Btw, this is the 3rd PR for the endpoint changes: https://github.com/guo0693/jaeger/pull/2/files, since the 3rd PR is relatively small, we can merge this PR first, I will create a patch from 3rd PR and apply to the merged master then open a new PR for endpoint changes.

So let's focus on getting this PR merged.

cmd/query/app/http_handler_test.go

cmd/query/app/querysvc/query_service.go

plugin/storage/cassandra/spanstore/reader_test.go

plugin/storage/grpc/shared/grpc_client_test.go

plugin/storage/grpc/shared/grpc_server_test.go

plugin/storage/integration/integration_test.go

plugin/storage/memory/memory.go

Signed-off-by: Jun Guo <guo0693@gmail.com>

plugin/storage/integration/integration_test.go

Signed-off-by: Jun Guo <guo0693@gmail.com>

guo0693 and others added 10 commits November 19, 2019 18:44

Create new cassandra table "operation_names_v2" with "spanKind" colum…

31ada18

…n for operation name index - add migration script - read from the latest table if available, otherwise fail back to previous table Signed-off-by: Jun Guo <guo0693@gmail.com>

Refactor schema versions

b9f7c2e

Signed-off-by: Jun Guo <guo0693@gmail.com>

Using typed constant for schemaVersion

6539d27

Signed-off-by: Jun Guo <guo0693@gmail.com>

Merge branch 'master' into jaegertracing#1921-2

4ee17f5

Fix travis build error

7e18806

Signed-off-by: Jun Guo <guo0693@gmail.com>

Merge branch 'jaegertracing#1921-2' of github.com:guo0693/jaeger into j…

ffae69d

…aegertracing#1921-2

Update storage api to support query operations by spanKind

e199801

Signed-off-by: Jun Guo <guo0693@gmail.com>

Wrap long lines and update proto to be backwards compatible

66c4389

Signed-off-by: Jun Guo <guo0693@gmail.com>

Merge branch 1921-2

1392f39

Signed-off-by: Jun Guo <guo0693@gmail.com>

Merge branch 'master' into jaegertracing#1921-3

ecba198

# Conflicts: # plugin/storage/cassandra/spanstore/operation_names.go # plugin/storage/cassandra/spanstore/operation_names_test.go # plugin/storage/cassandra/spanstore/reader_test.go # plugin/storage/cassandra/spanstore/writer_test.go

guo0693 requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners November 22, 2019 19:46

Fix line wrapping

17d9a78

Signed-off-by: Jun Guo <guo0693@gmail.com>

yurishkuro reviewed Nov 22, 2019

View reviewed changes

Address comment

3646ad9

- shorten var names and wrap lines - make grpc server backward compatible - write struct instead of list of strings for operation_names Signed-off-by: Jun Guo <guo0693@gmail.com>

yurishkuro reviewed Nov 22, 2019

View reviewed changes

- Pass by value instead of by pointer for GetOperations

ecf10f4

- Backward compatibility for grpc client Signed-off-by: Jun Guo <guo0693@gmail.com>

guo0693 force-pushed the #1921-3 branch from c6389e7 to ecf10f4 Compare November 23, 2019 00:40

yurishkuro reviewed Nov 25, 2019

View reviewed changes

Use operation as key for memory plugin

156af38

Signed-off-by: Jun Guo <guo0693@gmail.com>

yurishkuro reviewed Nov 25, 2019

View reviewed changes

plugin/storage/integration/integration_test.go Outdated Show resolved Hide resolved

fix integration test

81ca941

Signed-off-by: Jun Guo <guo0693@gmail.com>

yurishkuro approved these changes Nov 25, 2019

View reviewed changes

yurishkuro merged commit bf64373 into jaegertracing:master Nov 25, 2019

pavolloffay added this to the Release 1.16 milestone Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update storage api to support query operations by spanKind #1942

Update storage api to support query operations by spanKind #1942

guo0693 commented Nov 22, 2019 •

edited

Loading

yurishkuro left a comment

codecov bot commented Nov 22, 2019 •

edited

Loading

yurishkuro commented Nov 24, 2019

guo0693 commented Nov 25, 2019

guo0693 commented Nov 25, 2019 •

edited

Loading

Update storage api to support query operations by spanKind #1942

Update storage api to support query operations by spanKind #1942

Conversation

guo0693 commented Nov 22, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2019 • edited Loading

Codecov Report

yurishkuro commented Nov 24, 2019

guo0693 commented Nov 25, 2019

guo0693 commented Nov 25, 2019 • edited Loading

guo0693 commented Nov 22, 2019 •

edited

Loading

codecov bot commented Nov 22, 2019 •

edited

Loading

guo0693 commented Nov 25, 2019 •

edited

Loading