Skip to content

Include number of rows_affected in span db context for Sequel#668

Merged
mikker merged 6 commits intoelastic:masterfrom
estolfo:rows_affected
Feb 12, 2020
Merged

Include number of rows_affected in span db context for Sequel#668
mikker merged 6 commits intoelastic:masterfrom
estolfo:rows_affected

Conversation

@estolfo
Copy link
Copy Markdown
Contributor

@estolfo estolfo commented Jan 3, 2020 •
edited
Loading

Closes #574

Changes in this PR include:

  • Use start_span and end_span instead of with_span in the Sequel spy because the span's db context needs to be updated before it is queued for transport.
  • Add rows_affected attribute on Span::Context::DB object
  • Add rows_affected field in serialized span JSON object

** Opportunity for improvement: it'd be nice if we could somehow use the SQL summarizer to check if the statement is an update or delete to avoid using a regex here. Maybe we could return a parsed object instead of a String from the new SQL summarizer that you can call to_s on and other methods like update? or delete?.

@estolfo estolfo requested a review from mikker January 3, 2020 00:20
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 3, 2020 •
edited
Loading

Codecov Report

Merging #668 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #668   +/-   ##
=======================================
  Coverage   63.94%   63.94%           
=======================================
  Files          99       99           
  Lines        2951     2951           
=======================================
  Hits         1887     1887           
  Misses       1064     1064

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 3de9768...e11b1c9. Read the comment docs.

mikker
mikker approved these changes Jan 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mikker mikker left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good 👍

I like the idea of having a Sql value object with methods like you propose. This is probably also just fine for now.

Do we want to wait with merging this until it is merged (or maybe even released) in APM Server? APM Server will just ignore unknown fields so there's not going to be trouble but it might be a bit wasteful to send the bytes when no APM Server version will use them.

@estolfo
Copy link
Copy Markdown
Contributor Author

estolfo commented Jan 3, 2020

Yes, I agree this is fine for now. Maybe we can change the summarizer to return a value object when we've tested the new one enough and switch over to using it as the default.

I have no problem leaving this and waiting to merge until the server completes its implementation. 👍🏼

@mikker mikker merged commit df283cf into elastic:master Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mikker mikker mikker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Include number of rows_affected if available

3 participants

@estolfo @codecov-io @mikker