Skip to content

Conversation

@Kyle-Verhoog
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog commented Nov 15, 2018

The cassandra integration does not adhere spans created for batch statements to the specification required by the agent to obfuscate the query.

This PR changes the logic so that query is set as the resource of the span. Since the span type is already set to 'cassandra' the query can now be properly obfuscated by the agent.

Before:
screen shot 2018-11-15 at 3 07 02 pm

After:
screen shot 2018-11-15 at 2 47 20 pm

@Kyle-Verhoog Kyle-Verhoog added this to the 0.17.0 milestone Nov 15, 2018
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

I assume you tested this with agent obfuscation as well?

@Kyle-Verhoog
Copy link
Member Author

Yes this was tested with the agent obfuscation. However your comment made me check again and I caught an issue where including the query as a tag for a cassandra span will not result in the query being overwritten with the obfuscated query.

I've updated the PR description with the before and after of a trace.

One more thing to consider is if there are issues with using multiple combined queries as a resource.

@brettlangdon
Copy link
Member

Also, have you tested with running multiple queries at once in batch?

Either 2-3 of the same query or 2-3 of different queries. do we handle obfuscation properly there?

Other than that and the one question I had re: assertion, this appears to be mostly good to go.

@Kyle-Verhoog
Copy link
Member Author

Obfuscation seems to work with multiple queries, including different types of queries.

@brettlangdon
Copy link
Member

One last small question, but otherwise this is good to go.

@brettlangdon
Copy link
Member

There is a merge conflict here now as well.

@brettlangdon brettlangdon modified the milestones: 0.17.0, 0.17.1 Nov 26, 2018
@brettlangdon brettlangdon changed the base branch from 0.17-dev to 0.17.1-dev November 28, 2018 17:27
# reset query if a string is available
resource = getattr(query, "query_string", query)
elif t == 'BatchStatement':
resource = 'BatchStatement'
Copy link
Member

Choose a reason for hiding this comment

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

bad merge?

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

lgtm

@Kyle-Verhoog Kyle-Verhoog merged commit cb245cd into 0.17.1-dev Dec 3, 2018
@Kyle-Verhoog Kyle-Verhoog deleted the kyle-verhoog/fix-cassandra-leak branch December 3, 2018 19:46
Kyle-Verhoog added a commit that referenced this pull request Dec 5, 2018
Kyle-Verhoog added a commit that referenced this pull request Dec 5, 2018
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.

3 participants