feat(develop): Add SDK spec for database transactions#15194
feat(develop): Add SDK spec for database transactions#15194alexander-alderman-webb merged 16 commits intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
alexander-alderman-webb
changed the title
alexander-alderman-webb
left a comment
alexander-alderman-webb
left a comment
There was a problem hiding this comment.
Implemented your suggestions @lcian. Thanks!
| @@ -0,0 +1,21 @@ | |||
| --- | |||
| title: HTTP Requests within SQL Transactions Module | |||
There was a problem hiding this comment.
This spec doesn't cover HTTP, is the "HTTP Requests within" here intentional?
There was a problem hiding this comment.
I added the page in the modules folder here since the SQL transaction spans support a feature in the product. The page follows the same structure as the other pages in the folder, with a title of the feature we are supporting and the SDK requirements below.
As for whether our spec should be organized this way, that's probably another discussion we will have soon as the Sentry evolves.
There was a problem hiding this comment.
Just to be clear, there is no current product feature which detects this - that project was postponed as telemetry wasn't available. We are currently planning to use this information to detect a wide variety of SQL transaction related problems in Q4, including but not limited to HTTP requests within them
There was a problem hiding this comment.
Thanks, the clarification is useful. I didn't have the full context from the Linear project. I've moved the page and changed the title.
| Equivalent deprecated attributes MAY be used for backward compatibility, but they SHOULD be marked for deprecation. | ||
|
|
||
| The operation names for BEGIN, COMMIT and ROLLBACK instructions MUST be "BEGIN", "COMMIT", and "ROLLBACK", respectively. | ||
| For other transaction-level instructions like MySQL's START TRANSACTION, the SDK SHOULD represent the literal statement as closely as possible, omitting control-flow characters like semicolons. |
There was a problem hiding this comment.
Does this line also cover savepoint information, ex: SAVEPOINT, RELEASE SAVEPOINT, ROLLBACK TO, SAVE TRANSACTION?
There was a problem hiding this comment.
I would consider SAVEPOINT, RELEASE SAVEPOINT, ROLLBACK TO, SAVE TRANSACTION transaction-level statements, so they should have sensible operation name values that reflect the literal statement.
The spec is currently unclear what spans must be available for an SQL transaction to be considered instrumented.
What would be useful for you @mrduncan? Would tracing save points inside a transaction context enable better database issue detection?
I'll then amend the spec to be clearer about what database instructions should be traced.
There was a problem hiding this comment.
Would tracing save points inside a transaction context enable better database issue detection?
Yes, to some degree however it is totally reasonable for us to start focused on begin/commit/rollback for now as those are going to provide 90% of the value. If the others are present, great, but we can invest more in those in the future.
There was a problem hiding this comment.
Great to know, I've worked in that it's recommended to instrument all transaction-level statements. We'll still focus on BEGIN, COMMIT and ROLLBACK, and similar statements to begin with.
| title: SQL Transactions | ||
| --- | ||
|
|
||
| The SDK SHOULD auto-instrument all database transactions for SQL databases like PostgreSQL and MySQL. |
There was a problem hiding this comment.
| The SDK SHOULD auto-instrument all database transactions for SQL databases like PostgreSQL and MySQL. | |
| The SDK _should_ auto-instrument all database transactions for SQL databases like PostgreSQL and MySQL. |
| --- | ||
|
|
||
| The SDK SHOULD auto-instrument all database transactions for SQL databases like PostgreSQL and MySQL. | ||
| Each BEGIN, COMMIT, and ROLLBACK, or equivalent statement in the database system MUST result in an individual span. |
There was a problem hiding this comment.
| Each BEGIN, COMMIT, and ROLLBACK, or equivalent statement in the database system MUST result in an individual span. | |
| Each BEGIN, COMMIT, and ROLLBACK, or equivalent statement in the database system _must_ result in an individual span. |
|
|
||
| The SDK SHOULD auto-instrument all database transactions for SQL databases like PostgreSQL and MySQL. | ||
| Each BEGIN, COMMIT, and ROLLBACK, or equivalent statement in the database system MUST result in an individual span. | ||
| Spans for other transaction-level statements, such as SAVEPOINT, are RECOMMENDED and, when implemented, MUST also correspond to an individual SQL statement. |
There was a problem hiding this comment.
| Spans for other transaction-level statements, such as SAVEPOINT, are RECOMMENDED and, when implemented, MUST also correspond to an individual SQL statement. | |
| Spans for other transaction-level statements, such as SAVEPOINT, are _recommended_ and, when implemented, **must** also correspond to an individual SQL statement. |
| The SDK SHOULD auto-instrument all database transactions for SQL databases like PostgreSQL and MySQL. | ||
| Each BEGIN, COMMIT, and ROLLBACK, or equivalent statement in the database system MUST result in an individual span. | ||
| Spans for other transaction-level statements, such as SAVEPOINT, are RECOMMENDED and, when implemented, MUST also correspond to an individual SQL statement. | ||
| Transaction statements SHOULD produce spans whether they are issued through a database driver or through an ORM. |
There was a problem hiding this comment.
| Transaction statements SHOULD produce spans whether they are issued through a database driver or through an ORM. | |
| Transaction statements _should_ produce spans whether they are issued through a database driver or through an ORM. |
| ### Span Data | ||
|
|
||
| Refer to <Link to="https://getsentry.github.io/sentry-conventions/generated/attributes/db.html">Database Span Data Conventions</Link> for a full list of attributes that database spans should have. | ||
| The SDK SHOULD set the |
There was a problem hiding this comment.
| The SDK SHOULD set the | |
| The SDK should set the |
| - db.operation.name | ||
|
|
||
| attributes for transaction statement spans. | ||
| Equivalent deprecated attributes MAY be used for backward compatibility, but they SHOULD be marked for deprecation. |
There was a problem hiding this comment.
| Equivalent deprecated attributes MAY be used for backward compatibility, but they SHOULD be marked for deprecation. | |
| Equivalent deprecated attributes may be used for backward compatibility, but they _should_ be marked for deprecation. |
| attributes for transaction statement spans. | ||
| Equivalent deprecated attributes MAY be used for backward compatibility, but they SHOULD be marked for deprecation. | ||
|
|
||
| The operation names for BEGIN, COMMIT and ROLLBACK statements MUST be "BEGIN", "COMMIT", and "ROLLBACK", respectively. |
There was a problem hiding this comment.
| The operation names for BEGIN, COMMIT and ROLLBACK statements MUST be "BEGIN", "COMMIT", and "ROLLBACK", respectively. | |
| The operation names for BEGIN, COMMIT and ROLLBACK statements _must_ be "BEGIN", "COMMIT", and "ROLLBACK", respectively. |
| Equivalent deprecated attributes MAY be used for backward compatibility, but they SHOULD be marked for deprecation. | ||
|
|
||
| The operation names for BEGIN, COMMIT and ROLLBACK statements MUST be "BEGIN", "COMMIT", and "ROLLBACK", respectively. | ||
| For synonyms, like BEGIN TRANSACTION for BEGIN, the operation name MAY reflect the synonym. In this example, "BEGIN TRANSACTION" MAY be used. |
There was a problem hiding this comment.
| For synonyms, like BEGIN TRANSACTION for BEGIN, the operation name MAY reflect the synonym. In this example, "BEGIN TRANSACTION" MAY be used. | |
| For synonyms, like BEGIN TRANSACTION for BEGIN, the operation name may reflect the synonym. In this example, "BEGIN TRANSACTION" may be used. |
| The operation names for BEGIN, COMMIT and ROLLBACK statements MUST be "BEGIN", "COMMIT", and "ROLLBACK", respectively. | ||
| For synonyms, like BEGIN TRANSACTION for BEGIN, the operation name MAY reflect the synonym. In this example, "BEGIN TRANSACTION" MAY be used. | ||
| Some statements, like SAVEPOINT, ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT, take arguments. | ||
| The operation name for statements that accept arguments MUST be the statement without the arguments. For the examples above, the operation names are "SAVEPOINT", "ROLLBACK TO SAVEPOINT", and "RELEASE SAVEPOINT", respectively. |
There was a problem hiding this comment.
| The operation name for statements that accept arguments MUST be the statement without the arguments. For the examples above, the operation names are "SAVEPOINT", "ROLLBACK TO SAVEPOINT", and "RELEASE SAVEPOINT", respectively. | |
| The operation name for statements that accept arguments _must_ be the statement without the arguments. For the examples above, the operation names are "SAVEPOINT", "ROLLBACK TO SAVEPOINT", and "RELEASE SAVEPOINT", respectively. |
| Some statements, like SAVEPOINT, ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT, take arguments. | ||
| The operation name for statements that accept arguments MUST be the statement without the arguments. For the examples above, the operation names are "SAVEPOINT", "ROLLBACK TO SAVEPOINT", and "RELEASE SAVEPOINT", respectively. | ||
|
|
||
| For other transaction-level statements like MySQL's START TRANSACTION, the SDK SHOULD represent the literal statement as closely as possible, omitting control-flow characters like semicolons. |
There was a problem hiding this comment.
| For other transaction-level statements like MySQL's START TRANSACTION, the SDK SHOULD represent the literal statement as closely as possible, omitting control-flow characters like semicolons. | |
| For other transaction-level statements like MySQL's START TRANSACTION, the SDK should represent the literal statement as closely as possible, omitting control-flow characters like semicolons. |
sfanahata
left a comment
sfanahata
left a comment
There was a problem hiding this comment.
I'm confused about the consistent use of ALL CAPS for should, must, and may. It is more appropriate to use italics or bold to convey important words. This is also confusing because of the use of all caps for what I'm assuming are code blocks. Can we change these to begin, commit, rollback, etc? Is there a reason to leave them as caps?
|
There are two uses of all caps here @sfanahata:
It would be nice to standardize on whether we use uppercased modals in the docs, but that discussion is probably larger than the scope of this PR. |
|
@alexander-alderman-webb - Thanks for giving me the supporting docs to read. I'm not a huge fan of needing to explain what is being done on the page by providing secondary reading from an RFC. However, if this is commonly used formatting for SQL that we would expect developers to expect and understand, I support using it. I would suggest adding a header referencing the RFC, like found in the example doc from the PR you shared here. |
5 participants
DESCRIBE YOUR PR
Tell us what you're changing and why. If your PR resolves an issue, please link it so it closes automatically.
Spec out work for getsentry/sentry-python#4320.
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes:
LEGAL BOILERPLATE
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
EXTRA RESOURCES