Skip to content

Conversation

@SeanFeldman
Copy link
Contributor

@SeanFeldman SeanFeldman commented Jun 27, 2023

Fixes #1074

Add support to specify transaction idle timeout to finish an idling transaction.

@SeanFeldman SeanFeldman force-pushed the 1074-transacton-idle-timeout branch from dd874d2 to 1a03d78 Compare June 27, 2023 06:54
@jamescrosswell
Copy link
Collaborator

This could be achieved by extending TransactionTracer with an optional parameter for the timeout.

I imagine SDK users would want to do this by passing in a timeout via one of the Hub.StartTransaction extensions.

We probably need those additional extension methods before this functionality would be usable then right?

@SeanFeldman maybe worth showing how this would work in one of the sample projects, as a bit of a smoke test to make sure this is all wired up?

@SeanFeldman
Copy link
Contributor Author

SeanFeldman commented Jun 28, 2023

I imagine SDK users would want to do this by passing in a timeout via one of the Hub.StartTransaction extensions.

We probably need those additional extension methods before this functionality would be usable then right?

I don't know if it should be just an option or also an explicit option via extensions. For example, Java has it on SentryOptions

@bruno-garcia or @bitsandfoxes, do you have some context to share? Thank you.

@SeanFeldman
Copy link
Contributor Author

We probably need those additional extension methods before this functionality would be usable then right?

@jamescrosswell, yep. That will be the last step once I know everything that's needed is in place.

@bitsandfoxes
Copy link
Contributor

I imagine SDK users would want to do this by passing in a timeout via one of the Hub.StartTransaction extensions.
We probably need those additional extension methods before this functionality would be usable then right?

I don't know if it should be just an option or also an explicit option via extensions. For example, Java has it on SentryOptions

@bruno-garcia or @bitsandfoxes, do you have some context to share? Thank you.

I'm fine with having it as an explicit option. Maybe I'm missing something but what would be the benefit of putting it on the extension?

@bruno-garcia

This comment was marked as outdated.

@SeanFeldman
Copy link
Contributor Author

We probably need those additional extension methods before this functionality would be usable then right?

@jamescrosswell, yep. That will be the last step once I know everything that's needed is in place.

I believe these calls from a sample are import from the get go. Looking at what the API call will look like helps us go the right direction

I'm fine with having it as an explicit option. Maybe I'm missing something but what would be the benefit of putting it on the extension?

We have this as a default value in the options. Users shouldn't have to be concerned with that for the most part (and in fact, that's mainly used by us, on our auto instrumentation for UI frameworks). It's cheap to add overloads later but unless we're confident folks will need them to solve real use cases, default to leaving them out (API design 101: when in doubt leave them out)

@bruno-garcia, to confirm, you're saying we shouldn't add an extension method and stick to the initial SentryOption until the need emerges in something else. Is that accurate?

@SeanFeldman SeanFeldman requested a review from bruno-garcia July 5, 2023 05:37
@SeanFeldman

This comment was marked as resolved.

@bruno-garcia
Copy link
Member

I'll be honest, I'm concerned this is opened for 3 weeks and is still in draft. But I haven't really looked at it since my first round of comments. @bitsandfoxes what are the next steps here?

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@SeanFeldman SeanFeldman marked this pull request as ready for review July 19, 2023 00:34
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@SeanFeldman SeanFeldman requested a review from bitsandfoxes July 20, 2023 02:31
@bitsandfoxes
Copy link
Contributor

🚀

@bitsandfoxes bitsandfoxes merged commit bbd1e66 into main Jul 20, 2023
@bitsandfoxes bitsandfoxes deleted the 1074-transacton-idle-timeout branch July 20, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support transaction finishing automatically with 'idle timeout'

5 participants