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.

Sign up for GitHub

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

Jump to bottom

feat(tracing): Drop unfinished spans from a transaction #635

Closed
wants to merge 23 commits into from
Closed

feat(tracing): Drop unfinished spans from a transaction #635

wants to merge 23 commits into from

Conversation

relaxolotl
Copy link
Contributor

Remove all unfinished spans from transactions as they're being processed and sent over the wire to sentry. This addresses a TODO left behind in #634.

related to #601
picks up off of #634

@relaxolotl relaxolotl requested a review from a team December 18, 2021 06:28
@relaxolotl relaxolotl force-pushed the tracing/omit-unfinished-spans branch 2 times, most recently from ff00824 to 9e9ed29 Compare December 18, 2021 06:50
@relaxolotl relaxolotl mentioned this pull request Dec 18, 2021
Closed
@relaxolotl
feat(tracing): Add in basic Envelope support for Transactions
d223945
@relaxolotl
feat(tracing): Allow manual creation and sending of spanless Transact…
0ddd751 
…ions
@relaxolotl
rename sentry_start_transaction to sentry_transaction_start
204649f
@relaxolotl
brush up docstrings
61c6194
@relaxolotl
whoops
f47625d
@relaxolotl
made a mistake with no-setup
49ec6d8
@relaxolotl
no longer needed
c63c07b
@relaxolotl
todo cleanup
6b5d9fb
@relaxolotl
improve debug message
298c4a6
@relaxolotl
no more attachments for now
8cdd5c3
@relaxolotl
accidentally forgot to copy over a useful comment
3d44eeb
@relaxolotl
move uuid initialization into transactions
10ecee5
@relaxolotl
docs
ff9250e
@relaxolotl
return the event id of the transaction if successfully sent
d967e36
@relaxolotl
handle one subtle bug in capture event, note another possible bug
eec1502
@relaxolotl
this is already taken care of earlier in the pipeline
41f55a2
@relaxolotl
document ownership stealing
5c9d703
@relaxolotl
let's not pretend transaction contexts are transactions to avoid conf…
18d60d0 
…usion
@relaxolotl
feat(tracing): Only ever allow one active transaction, stuff transact…
ceddb01 
…ion into scope
@relaxolotl
no ambiguity allowed here
c87a8a0
@relaxolotl
feat(tracing): Defer some transaction validation and allow creation o…
cbf156a 
…f internal spans
@relaxolotl
feat(tracing): Basic span support with nesting
d07e1f0
@relaxolotl relaxolotl force-pushed the tracing/omit-unfinished-spans branch from 9e9ed29 to 8f8ee90 Compare December 21, 2021 08:33
Swatinem
Swatinem reviewed Dec 21, 2021
View reviewed changes
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

This ties in with my comment on the previous PR in this chain:
If we only ever record spans in finish (that means that start only creates the object but otherwise does nothing) this whole PR would become obsolete.

@relaxolotl relaxolotl mentioned this pull request Dec 23, 2021
Merged
@relaxolotl relaxolotl marked this pull request as draft December 23, 2021 06:20
@relaxolotl relaxolotl force-pushed the tracing/basic-spans branch 3 times, most recently from 0bec560 to 8969d44 Compare December 23, 2021 22:43
@relaxolotl relaxolotl force-pushed the tracing/basic-spans branch 2 times, most recently from 1dd00df to 3208c6a Compare January 7, 2022 07:57
@relaxolotl relaxolotl closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swatinem Swatinem Swatinem left review comments

@loewenheim loewenheim loewenheim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@relaxolotl @Swatinem @loewenheim