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

[ES, C*] Check for the presence of a tag #2054

Closed
wants to merge 2 commits into from

Conversation

annanay25
Copy link
Member

Which problem is this PR solving?

  • Adds functionality to check for the presence of a tag, to the ES and C* readers.

Short description of the changes

  • New queries for C* and ES backends.
  • Added parameter to the GRPC TraceQueryParameters and updated proto-generated files.
  • Added tests.

In the UI, this feature could be implemented as a search-box, input being a comma separated list of tags that need to be present in the result traces.

@annanay25 annanay25 requested a review from a team as a code owner February 3, 2020 12:25
@codecov
Copy link

codecov bot commented Feb 3, 2020

Codecov Report

Merging #2054 into master will decrease coverage by 0.13%.
The diff coverage is 51.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2054      +/-   ##
==========================================
- Coverage   97.39%   97.26%   -0.14%     
==========================================
  Files         207      207              
  Lines       10286    10330      +44     
==========================================
+ Hits        10018    10047      +29     
- Misses        223      238      +15     
  Partials       45       45
Impacted Files Coverage Δ
plugin/storage/cassandra/spanstore/reader.go 93.85% <0%> (-6.15%) ⬇️
cmd/query/app/grpc_handler.go 97.64% <100%> (+0.02%) ⬆️
plugin/storage/es/spanstore/reader.go 100% <100%> (ø) ⬆️
cmd/query/app/static_handler.go 86.84% <0%> (-1.76%) ⬇️
model/adjuster/clockskew.go 100% <0%> (ø) ⬆️
plugin/storage/badger/spanstore/reader.go 96.79% <0%> (ø) ⬆️
plugin/storage/kafka/options.go 93.68% <0%> (+1.47%) ⬆️
cmd/query/app/server.go 94.44% <0%> (+2.77%) ⬆️

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 0e974ce...76000b3. 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.

This needs to be added to integration tests.

@@ -301,9 +308,30 @@ func (s *SpanReader) findTraceIDs(ctx context.Context, traceQuery *spanstore.Tra
if len(traceQuery.Tags) > 0 {
return s.queryByTagsAndLogs(ctx, traceQuery)
}
if len(traceQuery.CheckTagsPresent) > 0 {
s.queryCheckTagPresence(ctx, traceQuery)
Copy link
Member

Choose a reason for hiding this comment

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

Return?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Will fix.

@annanay25
Copy link
Member Author

I'd like to wait for #2049 to be merged before moving forward with this.

@jpkrohling
Copy link
Contributor

@annanay25 looks like #2049 is already there ;-)

@jpkrohling jpkrohling closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants