feat: Add a method to start a transaction #482
Conversation
phacops
changed the title
phacops
changed the title
phacops
changed the title
tracing.go
Outdated
| func StartTransaction(ctx context.Context, name string, options ...SpanOption) *Span { | ||
| currentTransaction := ctx.Value(spanContextKey{}) | ||
| if currentTransaction != nil { | ||
| panic(ErrTransactionAlreadyInProgress) |
There was a problem hiding this comment.
Not sure if I like the idea of panicing here, we shouldn't cause possible runtime issues with our SDK methods.
Could we return err instead here? And have users handle this?
There was a problem hiding this comment.
Right, I can return an error instead here. I'll be back to asking if I can panic in the HTTP handler though, and so, maybe you can shed some light on how other SDKs are dealing with this if that's a case that can happen?
Could we silently return the existing transaction with a bool to say it already was in progress instead? Could we just return nil and the user would have test for the transaction or get the existing one themselves?
There was a problem hiding this comment.
Yeah so there are the two options
- We return
err, so the users have to grab the existing transaction themselves. - We return the active transaction and don't create one. This does mean that the transaction won't have the context supplied in the constructor.
Option 2 is more "magic" behavior, but I think it's correct since two transactions running at once is basically useless to a person, so let us maybe do that.
Could we silently return the existing transaction with a bool to say it already was in progress instead
Sounds good to me, I think that's cleaner API
|
In the end, the compromise was to return the existing transaction to avoid any |
|
Great work! So when will this feature be released as v0.15.0 of this package?? |
|
Yes @GoWithPlay, this will be part of the next release! |
|
@AbhiPrasad And when? The latest release took 7 months... too long
|
|
It'll be less than 7 months 😅, look forward to something within the next 2-3 weeks. |
|
@AbhiPrasad Sounds really great! |
|
@GoWithPlay |
More like 2 days! |
In the quest of having a similar API as the other SDKs, we wanted to add a
StartTransactionfunction to start a transaction.Since the Go SDK makes everything a span and the root span becomes the transaction, we don't need more than a function that calls
StartSpanin order to make something compatible with a big difference beingTransactionNameis now required instead ofOpandOpcan be set as an option.You might notice I chose to
panicif there's already a transaction in progress in the context. I'm not 100% sure how other SDKs are handling this case. We could choose to return the current transaction, stop the current transaction and start a new one instead of apanic. This will still means several transactions can happen at the same time, just with different contexts.Fixes #474.