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

Separate query-service functionality from http handler #1312

Merged

Conversation

Projects
None yet
2 participants
@annanay25
Copy link
Contributor

commented Feb 1, 2019

Signed-off-by: Annanay annanay.a@media.net

Which problem is this PR solving?

Short description of the changes

  • Separate common utils (span reader/writer, logger, tracer) of the query-service from the APIHandler definition. This will be used by the gRPC handler as well.
[query] Separate common utils of query-service from APIHandler
Signed-off-by: Annanay <annanay.a@media.net>
@codecov

This comment has been minimized.

Copy link

commented Feb 1, 2019

Codecov Report

Merging #1312 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1312   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         162     163    +1     
  Lines        7364    7374   +10     
======================================
+ Hits         7364    7374   +10
Impacted Files Coverage Δ
cmd/query/app/handler_options.go 100% <ø> (ø) ⬆️
cmd/query/app/http_handler.go 100% <100%> (ø)
cmd/query/app/querysvc/query_service.go 100% <100%> (ø)

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 df6e276...20859e6. Read the comment docs.

Show resolved Hide resolved cmd/query/app/query_service.go Outdated
Show resolved Hide resolved cmd/query/app/handler.go Outdated
Show resolved Hide resolved cmd/query/app/query_service.go Outdated
Make a new querysvc to encapsulate query-service utilities
Signed-off-by: Annanay <annanay.a@media.net>
@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

Thanks for the review, @yurishkuro

I've addressed comments and updated the PR. (A few failing tests will be fixed soon).

Show resolved Hide resolved cmd/query/app/querysvc/query_service.go Outdated
Show resolved Hide resolved cmd/query/app/querysvc/query_service.go Outdated
Show resolved Hide resolved cmd/query/app/querysvc/query_service_options.go Outdated
Show resolved Hide resolved cmd/query/app/http_handler.go Outdated
Addressed comments, refactored
Signed-off-by: Annanay <annanay.a@media.net>
@yurishkuro
Copy link
Member

left a comment

this looks pretty good, no major comments

Show resolved Hide resolved cmd/query/app/http_handler.go Outdated
Show resolved Hide resolved cmd/query/app/http_handler.go Outdated
Show resolved Hide resolved cmd/query/app/querysvc/query_service_options.go Outdated
Show resolved Hide resolved cmd/query/app/querysvc/query_service.go Outdated
Show resolved Hide resolved cmd/query/app/querysvc/query_service.go Outdated
Show resolved Hide resolved cmd/query/app/querysvc/query_service.go Outdated
Show resolved Hide resolved cmd/query/app/querysvc/query_service.go Outdated
Show resolved Hide resolved cmd/query/main.go Outdated
Show resolved Hide resolved cmd/query/app/http_handler.go Outdated
Cleaned function signatures, removed extra query_service_options file
Signed-off-by: Annanay <annanay.a@media.net>
@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

A couple of things I came across as I'm fixing the test cases after refactoring.

  1. Test cases like -
    func TestGetDependenciesSuccess(t *testing.T) {
    server, _, mock := initializeTestServer()
    defer server.Close()
    expectedDependencies := []model.DependencyLink{{Parent: "killer", Child: "queen", CallCount: 12}}
    endTs := time.Unix(0, 1476374248550*millisToNanosMultiplier)
    mock.On("GetDependencies", endTs, defaultDependencyLookbackDuration).Return(expectedDependencies, nil).Times(1)

... are going to change to -

func TestGetDependenciesSuccess(t *testing.T) {
	server, qs := initializeTestServer()
	defer server.Close()
	expectedDependencies := []model.DependencyLink{{Parent: "killer", Child: "queen", CallCount: 12}}
	endTs := time.Unix(0, 1476374248550*millisToNanosMultiplier)

And so we either need:

// Expose DependencyReader directly  
qs.DependencyReader.On("GetDependencies", endTs, defaultDependencyLookbackDuration).Return(expectedDependencies, nil).Times(1)

OR

// Expose dependencyReader through QueryService.getDepReader()
qs.getDepReader().On("GetDependencies", endTs, defaultDependencyLookbackDuration).Return(expectedDependencies, nil).Times(1)

.. (if we don't go with implementing a mock for QueryService)

  1. Since we're moving away from functional arguments, we'll now need constructors for QueryServiceOptions with signatures like -
// QueryServiceOptions has optional members of QueryService
type QueryServiceOptions struct {
	archiveSpanReader spanstore.Reader
	archiveSpanWriter spanstore.Writer
	adjuster          adjuster.Adjuster
}

// NewQueryServiceWithReader creates a QueryServiceOption with an archive reader.
func NewQueryServiceWithReader(archiveSpanReader spanstore.Reader) QueryServiceOptions

// NewQueryServiceWithWriter creates a QueryServiceOption with an archive writer.
func NewQueryServiceWithWriter(archiveSpanWriter spanstore.Writer) QueryServiceOptions 

.
.

@yurishkuro could you please review?

@yurishkuro

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

On the first question, initTestServer is the one that creates and returns the mocks for storage. It should continue doing that. The tests are testing the handler endpoints, they don't care if internally the init function constructs QueryService. It seems like I'm missing something that's confusing you.

On the second question, you need to make the fields of the options struct public, since they are part of the public API of the package.

Addressed comments, fixed tests
Signed-off-by: Annanay <annanay.a@media.net>
@yurishkuro
Copy link
Member

left a comment

a couple minor nits, ready to merge otherwise

Show resolved Hide resolved cmd/query/app/http_handler.go
Show resolved Hide resolved cmd/query/app/querysvc/query_service.go Outdated

annanay25 added some commits Feb 7, 2019

Addressed comments, fixed tests
Signed-off-by: Annanay <annanay.a@media.net>
Nit, fix tests
Signed-off-by: Annanay <annanay.a@media.net>
Run make format
Signed-off-by: Annanay <annanay.a@media.net>
@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@yurishkuro One test is failing because the querysvc directory doesn't have a test file. Not sure what to do here.

@yurishkuro
Copy link
Member

left a comment

One test is failing because the querysvc directory doesn't have a test file. Not sure what to do here.

ideally, we should add tests, it's a simple one to test with mocks.

Show resolved Hide resolved cmd/query/app/utils.go Outdated
Show resolved Hide resolved cmd/query/app/utils.go Outdated

annanay25 added some commits Feb 7, 2019

Rename file, remove alias
Signed-off-by: Annanay <annanay.a@media.net>
Add empty test for coverage
Signed-off-by: Annanay <annanay.a@media.net>

@annanay25 annanay25 referenced this pull request Feb 8, 2019

Closed

Release 1.10 #1321

4 of 4 tasks complete
@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

I've added an empty test for now because we have all the mock-tests in place for the reader/writer handlers anyway. Could you re-run travis on this @yurishkuro? The build seems to be failing with an unrelated error.

@wafflebot wafflebot bot added the review label Feb 10, 2019

@yurishkuro

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@annanay25 there is a drop in code coverage, which shouldn't happen with plain refactoring. Could you find a way to bring coverage back?

@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Looking into this.

annanay25 added some commits Feb 12, 2019

Remove unreferenced function QueryService.FindTraceIDs
Signed-off-by: Annanay <annanay.a@media.net>
WIP - Fix codecov coverage, add a few querysvc tests
Signed-off-by: Annanay <annanay.a@media.net>
Nit, fix imports
Signed-off-by: Annanay <annanay.a@media.net>

annanay25 added some commits Feb 14, 2019

Fix codecov coverage with tests for querysvc
Signed-off-by: Annanay <annanay.a@media.net>
Make format
Signed-off-by: Annanay <annanay.a@media.net>
Fix codecov coverage for APIHandler
Signed-off-by: Annanay <annanay.a@media.net>
@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

@yurishkuro Build is green :)

@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

@yurishkuro can we go ahead with this?

handler.RegisterRoutes(r)
return httptest.NewServer(r), readStorage, dependencyStorage, handler
}

func initializeTestServerWithQueryOptions(queryOptions querysvc.QueryServiceOptions, options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader) {

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Feb 18, 2019

Member

this is identical to initializeTestServerWithOptions, please remove

Remove duplicate function
Signed-off-by: Annanay <annanay.a@media.net>

@yurishkuro yurishkuro changed the title [query] Separate common utils of query-service from APIHandler Separate query-service functionality from http handler Feb 18, 2019

@yurishkuro yurishkuro merged commit ed6acfb into jaegertracing:master Feb 18, 2019

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (target 100%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the review label Feb 18, 2019

@yurishkuro

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

🎉 🎈

@annanay25

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Thanks Yuri! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.