event: suggestion: event.Start returns end
The majority of calls to start a trace span will be followed by a deferred call to end one:
ctx = event.To(ctx).Start(name)
defer event.To(ctx).End()
The main problem with this is that it is easy to forget that end must
be called. It's also more verbose than necessary.
Instead, have event.Start return both a context and a function that
can be deferred:
ctx, end := event.To(ctx).Start(name)
defer end()
The second return value will make it hard to forget the defer, and it
follows a common Go pattern.
You can also ignore it and call event.To(ctx).End() elsewhere if that
suits you.
Lastly, remove Builder.Start. It invites misuse by end users: they
should always do tracing with event.Start to properly construct the
span tree. (If you really want to do it, you can still call Deliver.)
Change-Id: Ibe7780ce5277e9d6dac5406e879a35d727a89eff
Reviewed-on: https://go-review.googlesource.com/c/exp/+/313409
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/event/bench/event_test.go b/event/bench/event_test.go
index 582adc8..a646de4 100644
--- a/event/bench/event_test.go
+++ b/event/bench/event_test.go
@@ -50,7 +50,7 @@
eventTrace = Hooks{
AStart: func(ctx context.Context, a int) context.Context {
- ctx = event.Start(ctx, aMsg)
+ ctx, _ = event.Start(ctx, aMsg)
event.To(ctx).With(aValue.Of(a)).Annotate()
return ctx
},
@@ -58,7 +58,7 @@
event.To(ctx).End()
},
BStart: func(ctx context.Context, b string) context.Context {
- ctx = event.Start(ctx, bMsg)
+ ctx, _ = event.Start(ctx, bMsg)
event.To(ctx).With(bValue.Of(b)).Annotate()
return ctx
},
diff --git a/event/builder.go b/event/builder.go
index 1b528f9..32f80d1 100644
--- a/event/builder.go
+++ b/event/builder.go
@@ -90,11 +90,6 @@
b.Deliver(LogKind, fmt.Sprintf(template, args...))
}
-// Start is a helper that calls Deliver with StartKind.
-func (b *Builder) Start(name string) uint64 {
- return b.Deliver(StartKind, name)
-}
-
// End is a helper that calls Deliver with EndKind.
func (b *Builder) End() {
b.Deliver(EndKind, "")
diff --git a/event/event_test.go b/event/event_test.go
index 1742163..665f31a 100644
--- a/event/event_test.go
+++ b/event/event_test.go
@@ -65,8 +65,8 @@
`}, {
name: "span",
events: func(ctx context.Context) {
- ctx = event.Start(ctx, "span")
- event.To(ctx).End()
+ ctx, end := event.Start(ctx, "span")
+ end()
},
expect: `
2020/03/05 14:27:48 [start:1] span
@@ -74,10 +74,10 @@
`}, {
name: "span nested",
events: func(ctx context.Context) {
- ctx = event.Start(ctx, "parent")
- defer func() { event.To(ctx).End() }()
- child := event.Start(ctx, "child")
- defer func() { event.To(child).End() }()
+ ctx, end := event.Start(ctx, "parent")
+ defer end()
+ child, end2 := event.Start(ctx, "child")
+ defer end2()
event.To(child).Log("message")
},
expect: `
diff --git a/event/export.go b/event/export.go
index 8f2ff09..8bb3f38 100644
--- a/event/export.go
+++ b/event/export.go
@@ -87,14 +87,22 @@
}
// Start delivers a start event and also updates the context with the event id.
-func Start(ctx context.Context, name string) context.Context {
+// Its second argument is a function that should be called to deliver the
+// matching end event. In lieue of calling the end function, you can invoke
+// event.To(ctx).End()
+// on the returned context.
+func Start(ctx context.Context, name string) (_ context.Context, end func()) {
b := To(ctx)
- if b.exporter == nil {
- return ctx
+ if b == nil || b.exporter == nil {
+ return ctx, func() {}
}
v := contextValue{exporter: b.exporter}
- v.parent = b.Start(name)
- return context.WithValue(ctx, contextKey{}, v)
+ v.parent = b.Deliver(StartKind, name)
+ return context.WithValue(ctx, contextKey{}, v), func() {
+ eb := v.exporter.Builder()
+ eb.Event.Parent = v.parent
+ eb.Deliver(EndKind, "")
+ }
}
// Deliver events to the underlying handler.
