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

Stream execute query logging #6056

Merged
merged 3 commits into from
Apr 13, 2020

Conversation

arka-g
Copy link
Collaborator

@arka-g arka-g commented Apr 12, 2020

Description

At Slack, we noticed that we're dropping logs for all queries of type STREAM_EXECUTE at the vttablet level. We run with the querylog-filter-tag option to determine what queries should be logged. The reason we were dropping all the logs for stream queries is because the Stream() codepath overwrites the OriginalSQL in logstats, which we rely on for checking if we should log this query here. This PR fixes that by not overwriting that field.

More details

The bug seems to be in the Stream vttablet method, we overwrite OriginalSQL to the version without the comments. In the callback that we pass to execRequest, we end up calling SplitMarginComments which strips comments before invoking Stream(). So, in the stream context, we don’t have the comments attached to the sql. This is fine because execRequest wraps the entire thing, and it has the full query with the comment.

I thought about two possible fixes:

  1. append the leading and trailing comments when overwriting OriginalSQL inside Stream()
  2. don’t overwrite OriginalSQL inside Stream(). this assignment seems unnecessary because execRequest initializes this properly and wraps the call. This PR implements this approach.

Testing

Added a unit test to ensure we preserve comments in the stream path.

@arka-g arka-g requested a review from sougou as a code owner April 12, 2020 21:47
@arka-g arka-g marked this pull request as draft April 12, 2020 21:49
@arka-g arka-g force-pushed the stream-execute-query-logging branch from c8b215b to f28b360 Compare April 12, 2020 21:53
@arka-g arka-g force-pushed the stream-execute-query-logging branch from f8892b1 to aa1dabf Compare April 12, 2020 22:12
@arka-g arka-g marked this pull request as ready for review April 12, 2020 22:55
@sougou sougou merged commit a40c5e7 into vitessio:master Apr 13, 2020
ajm188 pushed a commit to tinyspeck/vitess that referenced this pull request Apr 28, 2020
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