-
Notifications
You must be signed in to change notification settings - Fork 111
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
Include "number of rows affected" in the data captured for SQL commands #112
Comments
@SergeyKleyman just verifying- this is about SQL Data Manipulation Language (DML) statements ( |
@eyalkoren Yes, you are correct. |
SGTM, but can we shorten the name to just |
@axw If you think |
Should |
My vote would be to omit. |
Could we include the explain of slow traces from #84 ? |
We are currently implementing that in the Java agent. |
I don't see an issue with adding it as proposed on the Intake API. IMO the question to clarify for the server is which field to use in ES. There is an open issue to standardize SQL information in ECS. Some beats are already collecting this information for various modules, but store the information under non-standardized fields. |
@simitt Good point (as always 🙂), thanks! I wasn't aware of that. |
|
FYI, I started implementing this and immediately ran into issues. Some database drivers don't set the
Only |
We can certainly also discuss adding an additional field like |
Ah, right. Python's DB-API2 (which all relevant database drivers implement) uses the same attribute I'll update my implementation to only include the |
Not that I hate naming discussions as much as anybody, but @estolfo brings up an interesting point in the Ruby implementation issue: there are other data stores for which the number of items (for the lack of a better term) affected might be interesting. Using Shortening to |
The MongoDB server response field that the driver processes is actually |
I would prefer not using abbreviations. If |
|
Argh :| So...
Is it helpful to combine them? If we did that, we would be throwing away information (affected = modified+deleted) How about we keep |
Whatever we decide on, I think it'd probably have to be optional. As for joining or separating concerns of modified/deleted, I doubt you'd often see a single query do both, so the context could probably be derived from the query itself. It's harder to do aggregations on that way though. |
👍 All fields we add to an existing object at this point are optional.
Multi-document transactions aren't terribly unusual, e.g. atomically deleting one document and creating another. In the same vein, Elasticsearch's Update By Query API can be used to modify or delete multiple documents. |
Should we (re-)focus this issue on the SQL-case ( |
If everyone's on board with my suggestion, +1 to punting docs_* to another issue. |
True. This would get very unclear on a transaction. 🤔 |
Created #161. If anyone objects, please speak up. Thanks @estolfo and @beniwohli for identifying and flagging the issue! |
There were some naming suggestions here like I think the last suggestion was |
Yes, it's |
Implemented in the APM Server in elastic/apm-server#3095. |
closing this out as intake is complete and all agents have either implemented or have an issue open |
Description of the issue
Some SQL commands return the number of rows affected by the command. For example
.NET's SqlCommand.ExecuteNonQuery.
It might be useful for users to get this number as part of the data captured for the relevant SQL commands. For example requested at https://discuss.elastic.co/t/include-record-count-on-read-or-write/189110
Proposed solution
We should add a property (for example
number_of_rows_affected
) of typenumber
tocontext.db
object in intake API for agents to send captured number.If you have concerns about the proposed solution, let's discuss.
What we are voting on
@elastic/apm-agent-devs
The text was updated successfully, but these errors were encountered: