model: de-pointer Transaction.Sampled (#5746)

model.Transaction.Sampled is no longer a pointer type. Now we always set the Sampled flag when instantiating the Transaction object. If an Elastic APM agent does not report "sampled", then we now set the value to true when decoding the event, rather than at transformation time. OTel/Jaeger agents only ever send sampled events, so in processor/otel we always set Sampled to true.
elastic · Jul 20, 2021 · 375aa84 · 375aa84
1 parent 22b718f
commit 375aa84
Show file tree

Hide file tree

Showing 14 changed files with 67 additions and 54 deletions.
diff --git a/model/modeldecoder/rumv3/decoder.go b/model/modeldecoder/rumv3/decoder.go
@@ -797,7 +797,7 @@ func mapToTransactionModel(from *transaction, metadata *model.Metadata, reqTime
 	if from.Sampled.IsSet() {
 		sampled = from.Sampled.Val
 	}
-	out.Sampled = &sampled
+	out.Sampled = sampled
 	if from.SampleRate.IsSet() {
 		if from.SampleRate.Val > 0 {
 			out.RepresentativeCount = 1 / from.SampleRate.Val

diff --git a/model/modeldecoder/v2/decoder.go b/model/modeldecoder/v2/decoder.go
@@ -1108,7 +1108,7 @@ func mapToTransactionModel(from *transaction, metadata *model.Metadata, reqTime
 	if from.Sampled.IsSet() {
 		sampled = from.Sampled.Val
 	}
-	out.Sampled = &sampled
+	out.Sampled = sampled
 	if from.SampleRate.IsSet() {
 		if from.SampleRate.Val > 0 {
 			out.RepresentativeCount = 1 / from.SampleRate.Val

diff --git a/model/transaction.go b/model/transaction.go
@@ -55,7 +55,7 @@ type Transaction struct {
 	Duration       float64
 	Marks          TransactionMarks
 	Message        *Message
-	Sampled        *bool
+	Sampled        bool
 	SpanCount      SpanCount
 	Page           *Page
 	HTTP           *Http
@@ -112,9 +112,7 @@ func (e *Transaction) fields() common.MapStr {
 		}
 		fields.set("span_count", spanCount)
 	}
-	// TODO(axw) change Sampled to be non-pointer, and set its final value when
-	// instantiating the model type.
-	fields.set("sampled", e.Sampled == nil || *e.Sampled)
+	fields.set("sampled", e.Sampled)
 	return common.MapStr(fields)
 }
 

diff --git a/model/transaction_test.go b/model/transaction_test.go
@@ -33,7 +33,6 @@ import (
 func TestTransactionTransform(t *testing.T) {
 	id := "123"
 	result := "tx result"
-	sampled := false
 	dropped, startedSpans := 5, 14
 	name := "mytransaction"
 
@@ -48,7 +47,7 @@ func TestTransactionTransform(t *testing.T) {
 				"id":       "",
 				"type":     "",
 				"duration": common.MapStr{"us": 0},
-				"sampled":  true,
+				"sampled":  false,
 			},
 			Msg: "Empty Event",
 		},
@@ -62,7 +61,7 @@ func TestTransactionTransform(t *testing.T) {
 				"id":       id,
 				"type":     "tx",
 				"duration": common.MapStr{"us": 65980},
-				"sampled":  true,
+				"sampled":  false,
 			},
 			Msg: "SpanCount empty",
 		},
@@ -78,7 +77,7 @@ func TestTransactionTransform(t *testing.T) {
 				"type":       "tx",
 				"duration":   common.MapStr{"us": 65980},
 				"span_count": common.MapStr{"started": 14},
-				"sampled":    true,
+				"sampled":    false,
 			},
 			Msg: "SpanCount only contains `started`",
 		},
@@ -94,7 +93,7 @@ func TestTransactionTransform(t *testing.T) {
 				"type":       "tx",
 				"duration":   common.MapStr{"us": 65980},
 				"span_count": common.MapStr{"dropped": 5},
-				"sampled":    true,
+				"sampled":    false,
 			},
 			Msg: "SpanCount only contains `dropped`",
 		},
@@ -106,7 +105,7 @@ func TestTransactionTransform(t *testing.T) {
 				Result:    result,
 				Timestamp: time.Now(),
 				Duration:  65.98,
-				Sampled:   &sampled,
+				Sampled:   true,
 				SpanCount: SpanCount{Started: &startedSpans, Dropped: &dropped},
 			},
 			Output: common.MapStr{
@@ -116,7 +115,7 @@ func TestTransactionTransform(t *testing.T) {
 				"result":     "tx result",
 				"duration":   common.MapStr{"us": 65980},
 				"span_count": common.MapStr{"started": 14, "dropped": 5},
-				"sampled":    false,
+				"sampled":    true,
 			},
 			Msg: "Full Event",
 		},
@@ -172,6 +171,7 @@ func TestEventsTransformWithMetadata(t *testing.T) {
 		URL:       &URL{Original: url},
 		Custom:    common.MapStr{"foo.bar": "baz"},
 		Message:   &Message{QueueName: "routeUser"},
+		Sampled:   true,
 	}
 	event := txWithContext.toBeatEvent()
 	assert.Equal(t, event.Fields, common.MapStr{

diff --git a/processor/otel/consumer.go b/processor/otel/consumer.go
@@ -208,6 +208,7 @@ func (c *Consumer) convertSpan(
 			Timestamp: timestamp,
 			Duration:  durationMillis,
 			Name:      name,
+			Sampled:   true,
 			Outcome:   spanStatusOutcome(otelSpan.Status()),
 		}
 		translateTransaction(otelSpan, otelLibrary, metadata, &transactionBuilder{Transaction: transaction})
@@ -902,7 +903,7 @@ func addTransactionCtxToErr(transaction *model.Transaction, err *model.Error) {
 	err.URL = transaction.URL
 	err.Page = transaction.Page
 	err.Custom = transaction.Custom
-	err.TransactionSampled = transaction.Sampled
+	err.TransactionSampled = &transaction.Sampled
 	err.TransactionType = transaction.Type
 }
 

diff --git a/processor/otel/exceptions_test.go b/processor/otel/exceptions_test.go
@@ -111,12 +111,13 @@ Caused by: LowLevelException
 	expectedMetadata := languageOnlyMetadata("java")
 	transaction, errors := transformTransactionSpanEvents(t, "java", exceptionEvent1, exceptionEvent2)
 	assert.Equal(t, []*model.Error{{
-		Metadata:        expectedMetadata,
-		TraceID:         transaction.TraceID,
-		ParentID:        transaction.ID,
-		TransactionID:   transaction.ID,
-		TransactionType: transaction.Type,
-		Timestamp:       timestamp,
+		Metadata:           expectedMetadata,
+		TraceID:            transaction.TraceID,
+		ParentID:           transaction.ID,
+		TransactionID:      transaction.ID,
+		TransactionType:    transaction.Type,
+		TransactionSampled: newBool(true),
+		Timestamp:          timestamp,
 		Exception: &model.Exception{
 			Type:    "java.net.ConnectException.OSError",
 			Message: "Division by zero",
@@ -154,12 +155,13 @@ Caused by: LowLevelException
 			}},
 		},
 	}, {
-		Metadata:        expectedMetadata,
-		TraceID:         transaction.TraceID,
-		ParentID:        transaction.ID,
-		TransactionID:   transaction.ID,
-		TransactionType: transaction.Type,
-		Timestamp:       timestamp,
+		Metadata:           expectedMetadata,
+		TraceID:            transaction.TraceID,
+		ParentID:           transaction.ID,
+		TransactionID:      transaction.ID,
+		TransactionType:    transaction.Type,
+		TransactionSampled: newBool(true),
+		Timestamp:          timestamp,
 		Exception: &model.Exception{
 			Type:    "HighLevelException",
 			Message: "MidLevelException: LowLevelException",
@@ -305,12 +307,13 @@ func TestEncodeSpanEventsNonJavaExceptions(t *testing.T) {
 	require.Len(t, errors, 1)
 
 	assert.Equal(t, &model.Error{
-		Metadata:        languageOnlyMetadata("COBOL"),
-		TraceID:         transaction.TraceID,
-		ParentID:        transaction.ID,
-		TransactionID:   transaction.ID,
-		TransactionType: transaction.Type,
-		Timestamp:       timestamp,
+		Metadata:           languageOnlyMetadata("COBOL"),
+		TraceID:            transaction.TraceID,
+		ParentID:           transaction.ID,
+		TransactionID:      transaction.ID,
+		TransactionType:    transaction.Type,
+		TransactionSampled: newBool(true),
+		Timestamp:          timestamp,
 		Exception: &model.Exception{
 			Type:    "the_type",
 			Message: "the_message",

diff --git a/processor/otel/test_approved/transaction_jaeger_full.approved.json b/processor/otel/test_approved/transaction_jaeger_full.approved.json
@@ -113,6 +113,7 @@
             },
             "transaction": {
                 "id": "0000000041414646",
+                "sampled": true,
                 "type": "http_request"
             },
             "url": {
@@ -168,6 +169,7 @@
             },
             "transaction": {
                 "id": "0000000041414646",
+                "sampled": true,
                 "type": "http_request"
             },
             "url": {
@@ -225,6 +227,7 @@
             },
             "transaction": {
                 "id": "0000000041414646",
+                "sampled": true,
                 "type": "http_request"
             },
             "url": {
@@ -282,6 +285,7 @@
             },
             "transaction": {
                 "id": "0000000041414646",
+                "sampled": true,
                 "type": "http_request"
             },
             "url": {
@@ -339,6 +343,7 @@
             },
             "transaction": {
                 "id": "0000000041414646",
+                "sampled": true,
                 "type": "http_request"
             },
             "url": {
@@ -394,6 +399,7 @@
             },
             "transaction": {
                 "id": "0000000041414646",
+                "sampled": true,
                 "type": "http_request"
             },
             "url": {

diff --git a/sampling/sampling.go b/sampling/sampling.go
@@ -39,7 +39,7 @@ func NewDiscardUnsampledBatchProcessor() model.BatchProcessor {
 		events := *batch
 		for i := 0; i < len(events); {
 			event := events[i]
-			if event.Transaction == nil || event.Transaction.Sampled == nil || *event.Transaction.Sampled {
+			if event.Transaction == nil || event.Transaction.Sampled {
 				i++
 				continue
 			}

diff --git a/sampling/sampling_test.go b/sampling/sampling_test.go
@@ -31,32 +31,28 @@ import (
 func TestNewDiscardUnsampledBatchProcessor(t *testing.T) {
 	batchProcessor := sampling.NewDiscardUnsampledBatchProcessor()
 
-	t1 := &model.Transaction{}
-	t2 := &model.Transaction{Sampled: newBool(false)}
-	t3 := &model.Transaction{Sampled: newBool(true)}
+	t1 := &model.Transaction{Sampled: false}
+	t2 := &model.Transaction{Sampled: true}
 	span := &model.Span{}
-	t4 := &model.Transaction{Sampled: newBool(false)}
-	t5 := &model.Transaction{Sampled: newBool(true)}
+	t3 := &model.Transaction{Sampled: false}
+	t4 := &model.Transaction{Sampled: true}
 
 	batch := model.Batch{
 		{Transaction: t1},
 		{Transaction: t2},
 		{Span: span},
 		{Transaction: t3},
 		{Transaction: t4},
-		{Transaction: t5},
 	}
 
 	err := batchProcessor.ProcessBatch(context.Background(), &batch)
 	assert.NoError(t, err)
 
-	// Note that t3 gets sent to the back of the slice;
-	// this reporter is not order-preserving.
+	// Note: this processor is not order-preserving.
 	assert.Equal(t, model.Batch{
-		{Transaction: t1},
-		{Transaction: t5},
+		{Transaction: t4},
+		{Transaction: t2},
 		{Span: span},
-		{Transaction: t3},
 	}, batch)
 
 	expectedMonitoring := monitoring.MakeFlatSnapshot()
@@ -69,7 +65,3 @@ func TestNewDiscardUnsampledBatchProcessor(t *testing.T) {
 	)
 	assert.Equal(t, expectedMonitoring, snapshot)
 }
-
-func newBool(v bool) *bool {
-	return &v
-}
diff --git a/testdata/jaeger/batch_0.approved.json b/testdata/jaeger/batch_0.approved.json
@@ -90,6 +90,7 @@
             },
             "transaction": {
                 "id": "7be2fd98d0973be3",
+                "sampled": true,
                 "type": "custom"
             }
         },
@@ -135,6 +136,7 @@
             },
             "transaction": {
                 "id": "7be2fd98d0973be3",
+                "sampled": true,
                 "type": "custom"
             }
         },
@@ -180,6 +182,7 @@
             },
             "transaction": {
                 "id": "7be2fd98d0973be3",
+                "sampled": true,
                 "type": "custom"
             }
         }

diff --git a/tests/system/jaeger_batch_0_auth_tag_removed.approved.json b/tests/system/jaeger_batch_0_auth_tag_removed.approved.json
@@ -125,6 +125,7 @@
         },
         "transaction": {
             "id": "7be2fd98d0973be3",
+            "sampled": true,
             "type": "custom"
         }
     },
@@ -190,6 +191,7 @@
         },
         "transaction": {
             "id": "7be2fd98d0973be3",
+            "sampled": true,
             "type": "custom"
         }
     },
@@ -255,6 +257,7 @@
         },
         "transaction": {
             "id": "7be2fd98d0973be3",
+            "sampled": true,
             "type": "custom"
         }
     }

diff --git a/tests/system/jaeger_batch_0_authorization.approved.json b/tests/system/jaeger_batch_0_authorization.approved.json
@@ -125,6 +125,7 @@
         },
         "transaction": {
             "id": "7be2fd98d0973be3",
+            "sampled": true,
             "type": "custom"
         }
     },
@@ -190,6 +191,7 @@
         },
         "transaction": {
             "id": "7be2fd98d0973be3",
+            "sampled": true,
             "type": "custom"
         }
     },
@@ -255,6 +257,7 @@
         },
         "transaction": {
             "id": "7be2fd98d0973be3",
+            "sampled": true,
             "type": "custom"
         }
     }

diff --git a/x-pack/apm-server/sampling/processor.go b/x-pack/apm-server/sampling/processor.go
@@ -202,7 +202,7 @@ func (p *Processor) updateProcessorMetrics(report, stored bool) {
 }
 
 func (p *Processor) processTransaction(tx *model.Transaction) (report, stored bool, _ error) {
-	if tx.Sampled != nil && !*tx.Sampled {
+	if !tx.Sampled {
 		// (Head-based) unsampled transactions are passed through
 		// by the tail sampler.
 		return true, false, nil