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

Remove unnecessary ServiceName index seek if tags query is available #2535

Merged

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Oct 4, 2020

Which problem is this PR solving?

  • Reduce the amount of queries against the badger backend store if Tags are used as a query parameter. Also, if the Tags is the only query, then we also remove single merge-join phase.

Short description of the changes

  • Tags' key already includes the ServiceName so it makes no sense to add the ServiceName index seek as we can filter it all in a single index seek.

@burmanm burmanm requested a review from a team as a code owner October 4, 2020 10:56
@burmanm burmanm requested a review from vprithvi October 4, 2020 10:56
@burmanm burmanm force-pushed the badger_unnecessary_index_seek branch from 624bce2 to 7b8d203 Compare October 4, 2020 10:57
@codecov
Copy link

codecov bot commented Oct 4, 2020

Codecov Report

Merging #2535 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
- Coverage   95.41%   95.38%   -0.04%     
==========================================
  Files         208      208              
  Lines        9219     9222       +3     
==========================================
  Hits         8796     8796              
- Misses        348      350       +2     
- Partials       75       76       +1     
Impacted Files Coverage Δ
plugin/storage/badger/spanstore/reader.go 95.37% <100.00%> (-0.67%) ⬇️
...lugin/sampling/strategystore/adaptive/processor.go 99.07% <0.00%> (-0.93%) ⬇️
plugin/storage/integration/integration.go 77.90% <0.00%> (+0.55%) ⬆️

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 0247828...7b8d203. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane to me. Just to confirm: this is covered with test cases already, right?

@burmanm
Copy link
Contributor Author

burmanm commented Oct 5, 2020

Multiple tests have Tags-queries, yes. The TestRandomTraceID fetches a query with Tags & ServiceName parameters and as such tests this code path with just the Tags-query being executed.

@jpkrohling jpkrohling merged commit 4f3b832 into jaegertracing:master Oct 5, 2020
@jpkrohling
Copy link
Contributor

Nice to see a PR from you :-)

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.

2 participants