Merge pull request newrelic#37 from will/another-segments

segments refactor

CHANGELOG.md

@@ -1,5 +1,12 @@
 ## ChangeLog
 
+* Breaking Segments Refactor:  The segments API has been rewritten with the goal
+  of being easier to use.  See:
+
+  * [segments.go](segments.go)
+  * [example/main.go](example/main.go)
+  * [GUIDE.md#segments](GUIDE.md#segments)
+
 ## 0.7.1
 
 * Fixed a bug causing the `Config` to fail to serialize into JSON when the

GUIDE.md

@@ -150,57 +150,67 @@ func myHandler(w http.ResponseWriter, r *http.Request) {
 Find out where the time in your transactions is being spent!  Each transaction
 should only track segments in a single goroutine.
 
-`Transaction` has methods to time external calls, datastore calls, functions,
-and arbitrary blocks of code.  
-
 To time a function, add the following line to the beginning of that function:
 
 ```go
-defer txn.EndSegment(txn.StartSegment(), "mySegmentName")
+defer newrelic.StartSegment(txn, "mySegmentName").End()
 ```
 
-The `defer` pattern will execute the `txn.StartSegment()` when this line is
-encountered and the `EndSegment()` method when this function returns.  More
-information can be found on `defer` [here](https://gobyexample.com/defer).
+`StartSegment` is safe even if the transaction is nil.
 
 To time a block of code, use the following pattern:
 
 ```go
-token := txn.StartSegment()
+segment := newrelic.StartSegment(txn, "mySegmentName")
 // ... code you want to time here ...
-txn.EndSegment(token, "mySegmentName")
+segment.End()
 ```
 
 Segments may be nested.  The segment being ended must be the most recently
 started segment.
 
 ```go
-token1 := txn.StartSegment()
-token2 := txn.StartSegment()
-// token2 must be ended before token1
-txn.EndSegment(token2, "innerSegment")
-txn.EndSegment(token1, "outerSegment")
+s1 := newrelic.StartSegment(txn, "outerSegment")
+s2 := newrelic.StartSegment(txn, "innerSegment")
+// s2 must be ended before s1
+s2.End()
+s1.End()
+```
+
+A zero value segment may safely be ended.  Therefore, the following code
+is safe even if the conditional fails:
+
+```go
+var s newrelic.Segment
+if txn, ok := w.(newrelic.Transaction); ok {
+	s.StartTime = newrelic.StartSegmentNow(txn),
+}
+// ... code you wish to time here ...
+s.End()
 ```
 
 ### Datastore Segments
 
 Datastore segments appear in the transaction "Breakdown table" and in the
-"Databases" tab.  They are finished using `EndDatastore`.  This requires
-importing the `datastore` subpackage.
+"Databases" tab.
 
 * [datastore.go](datastore/datastore.go)
 * [More info on Databases tab](https://docs.newrelic.com/docs/apm/applications-menu/monitoring/databases-slow-queries-page)
 
+To instrument a datastore call that spans an entire function call, add this
+to the beginning to the function:
+
 ```go
-defer txn.EndDatastore(txn.StartSegment(), datastore.Segment{
+defer newrelic.DatastoreSegment{
+	StartTime: newrelic.StartSegmentNow(txn),
 	// Product is the datastore type.
 	// See the constants in datastore/datastore.go.
 	Product: datastore.MySQL,
 	// Collection is the table or group.
 	Collection: "my_table",
 	// Operation is the relevant action, e.g. "SELECT" or "GET".
 	Operation: "SELECT",
-})
+}).End()
 ```
 
 ### External Segments
@@ -210,30 +220,37 @@ External segments appear in the transaction "Breakdown table" and in the
 
 * [More info on External Services tab](https://docs.newrelic.com/docs/apm/applications-menu/monitoring/external-services-page)
 
-There are a couple of ways to instrument external
-segments.  The simplest way is to use `EndExternal`:
+Populate either the `URL` or `Request` field of the `ExternalSegment` parameter
+to indicate the endpoint.
 
 ```go
-func externalCall(url string, txn newrelic.Transaction) (*http.Response, error) {
-	defer txn.EndExternal(txn.StartSegment(), url)
+func external(txn newrelic.Transaction, url string) (*http.Response, error) {
+	defer newrelic.ExternalFields{
+		Start: newrelic.StartSegmentNow(txn),
+		URL:   url,
+	}).End()
 
 	return http.Get(url)
 }
 ```
 
-The functions `PrepareRequest` and `EndRequest` are recommended since they will
-be used in the future to trace activity between distributed applications using
-headers.
+We recommend using the `Request` and `Response` fields of `ExternalFields` since
+they will be used in the future to trace activity between your New Relic
+applications using headers.  `Request` is populated automatically by the
+`StartExternalSegment` helper.
 
 ```go
-token := txn.StartSegment()
-txn.PrepareRequest(token, request)
-response, err := client.Do(request)
-txn.EndRequest(token, request, response)
+func external(txn newrelic.Transaction, req *http.Request) (*http.Response, error) {
+	s := newrelic.StartExternalSegment(txn, req)
+	response, err := http.DefaultClient.Do(req)
+	s.Response = response
+	s.End()
+	return response, err
+}
 ```
 
-`NewRoundTripper` is a helper built on top of `PrepareRequest` and `EndRequest`.
-This round tripper **must** be used the same goroutine as the transaction.
+`NewRoundTripper` is another useful helper.  The round tripper returned **must**
+only be used the same goroutine as the transaction.
 
 ```go
 client := &http.Client{}

README.md

@@ -85,7 +85,7 @@ Segments show you where time in your transactions is being spent.  At the
 beginning of important functions, add:
 
 ```go
-defer txn.EndSegment(txn.StartSegment(), "mySegmentName")
+defer newrelic.StartSegment(txn, "mySegmentName").End()
 ```
 
 [more info](GUIDE.md#segments), [segments.go](segments.go)

datastore/datastore.go

@@ -1,16 +1,5 @@
 package datastore
 
-// Segment contains the fields that should be provided when calling
-// Transaction.EndDatastore.
-type Segment struct {
-	// Product is the datastore type.  See the constants below.
-	Product Product
-	// Collection is the table or group.
-	Collection string
-	// Operation is the relevant action, e.g. "SELECT" or "GET".
-	Operation string
-}
-
 // Product encourages consistent metrics across New Relic agents.  You may
 // create your own if your datastore is not listed below.
 type Product string

example/main.go

@@ -83,14 +83,14 @@ func ignore(w http.ResponseWriter, r *http.Request) {
 }
 
 func segments(w http.ResponseWriter, r *http.Request) {
+	txn, _ := w.(newrelic.Transaction)
+
 	func() {
-		if txn, ok := w.(newrelic.Transaction); ok {
-			defer txn.EndSegment(txn.StartSegment(), "f1")
-		}
+		defer newrelic.StartSegment(txn, "f1").End()
+
 		func() {
-			if txn, ok := w.(newrelic.Transaction); ok {
-				defer txn.EndSegment(txn.StartSegment(), "f2")
-			}
+			defer newrelic.StartSegment(txn, "f2").End()
+
 			io.WriteString(w, "segments!")
 			time.Sleep(10 * time.Millisecond)
 		}()
@@ -100,23 +100,29 @@ func segments(w http.ResponseWriter, r *http.Request) {
 }
 
 func mysql(w http.ResponseWriter, r *http.Request) {
-	if txn, ok := w.(newrelic.Transaction); ok {
-		defer txn.EndDatastore(txn.StartSegment(), datastore.Segment{
-			Product:    datastore.MySQL,
-			Collection: "my_table",
-			Operation:  "SELECT",
-		})
-	}
+	txn, _ := w.(newrelic.Transaction)
+	defer newrelic.DatastoreSegment{
+		StartTime:  newrelic.StartSegmentNow(txn),
+		Product:    datastore.MySQL,
+		Collection: "my_table",
+		Operation:  "SELECT",
+	}.End()
 
 	time.Sleep(20 * time.Millisecond)
 	io.WriteString(w, `performing fake query "SELECT * from my_table"`)
 }
 
 func external(w http.ResponseWriter, r *http.Request) {
 	url := "http://example.com/"
-	if txn, ok := w.(newrelic.Transaction); ok {
-		defer txn.EndExternal(txn.StartSegment(), url)
-	}
+	txn, _ := w.(newrelic.Transaction)
+	// This demonstrates an external segment where only the URL is known. If
+	// an http.Request is accessible then `StartExternalSegment` is
+	// recommended. See the implementation of `NewRoundTripper` for an
+	// example.
+	defer newrelic.ExternalSegment{
+		StartTime: newrelic.StartSegmentNow(txn),
+		URL:       url,
+	}.End()
 
 	resp, err := http.Get(url)
 	if nil != err {
@@ -129,9 +135,8 @@ func external(w http.ResponseWriter, r *http.Request) {
 
 func roundtripper(w http.ResponseWriter, r *http.Request) {
 	client := &http.Client{}
-	if txn, ok := w.(newrelic.Transaction); ok {
-		client.Transport = newrelic.NewRoundTripper(txn, nil)
-	}
+	txn, _ := w.(newrelic.Transaction)
+	client.Transport = newrelic.NewRoundTripper(txn, nil)
 	resp, err := client.Get("http://example.com/")
 	if nil != err {
 		io.WriteString(w, err.Error())

instrumentation.go

@@ -49,15 +49,16 @@ func WrapHandleFunc(app Application, pattern string, handler func(http.ResponseW
 //
 func NewRoundTripper(txn Transaction, original http.RoundTripper) http.RoundTripper {
 	return roundTripperFunc(func(request *http.Request) (*http.Response, error) {
-		token := txn.StartSegment()
-		txn.PrepareRequest(token, request)
+		segment := StartExternalSegment(txn, request)
 
 		if nil == original {
 			original = http.DefaultTransport
 		}
 		response, err := original.RoundTrip(request)
 
-		txn.EndRequest(token, request, response)
+		segment.Response = response
+		segment.End()
+
 		return response, err
 	})
 }

internal.go

@@ -15,7 +15,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/newrelic/go-agent/datastore"
 	"github.com/newrelic/go-agent/internal"
 	"github.com/newrelic/go-agent/internal/logger"
 	"github.com/newrelic/go-agent/internal/utilization"
@@ -483,49 +482,72 @@ func (txn *txn) Ignore() error {
 	return nil
 }
 
-func (txn *txn) StartSegment() Token {
-	token := Token(0)
+func (txn *txn) StartSegmentNow() SegmentStartTime {
+	token := internal.Token(0)
 	txn.Lock()
 	if !txn.finished {
-		token = Token(internal.StartSegment(&txn.tracer, time.Now()))
+		token = internal.StartSegment(&txn.tracer, time.Now())
 	}
 	txn.Unlock()
-	return token
+	return SegmentStartTime{
+		segment: segment{
+			token: token,
+			txn:   txn,
+		},
+	}
+}
+
+type segment struct {
+	token internal.Token
+	txn   *txn
 }
 
-func (txn *txn) EndSegment(token Token, name string) {
+func endSegment(s Segment) {
+	txn := s.StartTime.txn
+	if nil == txn {
+		return
+	}
 	txn.Lock()
 	if !txn.finished {
-		internal.EndBasicSegment(&txn.tracer, internal.Token(token), time.Now(), name)
+		internal.EndBasicSegment(&txn.tracer, s.StartTime.token, time.Now(), s.Name)
 	}
 	txn.Unlock()
 }
 
-func (txn *txn) EndDatastore(token Token, s datastore.Segment) {
+func endDatastore(s DatastoreSegment) {
+	txn := s.StartTime.txn
+	if nil == txn {
+		return
+	}
 	txn.Lock()
 	defer txn.Unlock()
 
 	if txn.finished {
 		return
 	}
-	internal.EndDatastoreSegment(&txn.tracer, internal.Token(token), time.Now(), s)
+	internal.EndDatastoreSegment(&txn.tracer, s.StartTime.token, time.Now(), internal.DatastoreMetricKey{
+		Product:    s.Product,
+		Collection: s.Collection,
+		Operation:  s.Operation,
+	})
 }
 
-func (txn *txn) EndExternal(token Token, url string) {
+func endExternal(s ExternalSegment) {
+	txn := s.StartTime.txn
+	if nil == txn {
+		return
+	}
 	txn.Lock()
 	defer txn.Unlock()
 
 	if txn.finished {
 		return
 	}
-	internal.EndExternalSegment(&txn.tracer, internal.Token(token), time.Now(), internal.HostFromExternalURL(url))
-}
-
-func (txn *txn) PrepareRequest(token Token, request *http.Request) {
-	txn.Lock()
-	defer txn.Unlock()
-
-	// TODO: handle request CAT headers
+	host := hostFromRequestResponse(s.Request, s.Response)
+	if "" != s.URL {
+		host = internal.HostFromExternalURL(s.URL)
+	}
+	internal.EndExternalSegment(&txn.tracer, s.StartTime.token, time.Now(), host)
 }
 
 func hostFromRequestResponse(request *http.Request, response *http.Response) string {
@@ -541,20 +563,6 @@ func hostFromRequestResponse(request *http.Request, response *http.Response) str
 	return request.URL.Host
 }
 
-func (txn *txn) EndRequest(token Token, request *http.Request, response *http.Response) {
-	txn.Lock()
-	defer txn.Unlock()
-
-	if txn.finished {
-		return
-	}
-
-	// TODO: handle response CAT headers
-
-	host := hostFromRequestResponse(request, response)
-	internal.EndExternalSegment(&txn.tracer, internal.Token(token), time.Now(), host)
-}
-
 type appData struct {
 	id   internal.AgentRunID
 	data internal.Harvestable