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.

Sign up for GitHub

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

Jump to bottom

Add C* query to error logs #1250

Merged
merged 2 commits into from
Jan 1, 2019
Merged

Add C* query to error logs #1250

merged 2 commits into from
Jan 1, 2019

Conversation

vprithvi
Copy link
Contributor

Which problem is this PR solving?

  • Determine which C* caused a read to fail

Short description of the changes

  • Add query details to the log. Gocql should log these as [query statement=%q values=%+v consistency=%s]

@ghost ghost assigned vprithvi Dec 13, 2018
@ghost ghost added the review label Dec 13, 2018
@vprithvi
Copy link
Contributor Author

Relates to #1249

@vprithvi vprithvi force-pushed the log-failing-queries branch from da64066 to 7d82b97 Compare December 13, 2018 19:04
black-adder
black-adder reviewed Dec 13, 2018
View reviewed changes
Copy link
Contributor

@black-adder black-adder left a comment

Choose a reason for hiding this comment

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

isn't the problem here that the error isn't being percolated all the way back to the UI? Also, I love how we're using jaeger to debug jaeger UI issues, that's so meta.

@vprithvi
Copy link
Contributor Author

isn't the problem here that the error isn't being percolated all the way back to the UI?

I don't think so - the error message from C*, as seen in #1249 is Operation failed - received 0 responses and 1 failures. As far as I can tell, there is no additional information on the query object in the version of gocql we are using.

@black-adder
Copy link
Contributor

isn't the problem here that the error isn't being percolated all the way back to the UI?

I don't think so - the error message from C*, as seen in #1249 is Operation failed - received 0 responses and 1 failures. As far as I can tell, there is no additional information on the query object in the version of gocql we are using.

Odd, no where in the gocql codebase can I see how the error Operation failed - received 0 responses and 1 failures could be generated. That aside, do we want to wrap the error that we return from the cassandra writer with the error message in the gocql query object?

@vprithvi
Copy link
Contributor Author

vprithvi commented Dec 13, 2018 •
edited
Loading

Odd, no where in the gocql codebase can I see how the error Operation failed - received 0 responses and 1 failures could be generated.

This is because it is generated on Cassandra and set to jaeger-query via the gocql error object. gocql is simply the conduit.

@vprithvi
Add query to read error log
cba3c9c 
Signed-off-by: Prithvi Raj <p.r@uber.com>
@vprithvi vprithvi force-pushed the log-failing-queries branch from 7d82b97 to cba3c9c Compare December 13, 2018 19:35
@codecov
Copy link

codecov bot commented Dec 13, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1250   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         160     160           
  Lines        7181    7182    +1     
======================================
+ Hits         7181    7182    +1
Impacted Files Coverage Δ
plugin/storage/cassandra/spanstore/reader.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 eed786f...f77566d. Read the comment docs.

yurishkuro
yurishkuro reviewed Dec 13, 2018
View reviewed changes
plugin/storage/cassandra/spanstore/reader.go
@@ -410,7 +410,8 @@ func (s *SpanReader) executeQuery(span opentracing.Span, query cassandra.Query,
tableMetrics.Emit(err, time.Since(start))
if err != nil {
logErrorToSpan(span, err)
s.logger.Error("Failed to exec query", zap.Error(err))
span.LogFields(otlog.String("query", query.String()))
s.logger.Error("Failed to exec query", zap.Error(err), zap.String("query", query.String()))
Copy link
Member

Choose a reason for hiding this comment

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

zap.String("query", query.String()) => zap.Stringer("query", query)

@ghost ghost assigned yurishkuro Jan 1, 2019
@yurishkuro yurishkuro merged commit fe7352d into master Jan 1, 2019
@ghost ghost removed the review label Jan 1, 2019
@yurishkuro yurishkuro deleted the log-failing-queries branch January 3, 2019 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@black-adder black-adder black-adder approved these changes

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

@objectiser objectiser Awaiting requested review from objectiser

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tiffon tiffon Awaiting requested review from tiffon

Assignees

@vprithvi vprithvi

@yurishkuro yurishkuro

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vprithvi @black-adder @yurishkuro