model/metadata: make all fields non-pointer types (#3618) (#3635)

The primary motivation behind this change is to lay the groundwork for merging shared (i.e. stream) and per-event metadata at decode time, rather than transformation time, which we'll need for #3485.

We could merge metadata without these changes, but it would be more difficult and error prone. Making these change also provide some performance improvements – see below. Finally, there is also overlap between merging metadata and revising the decoders to enable memory use (#3551 (comment)).

In theory this could be a considered a breaking change, due to the fact that an empty string coming from an agent would no longer be recorded in output documents. In practice, it does not make sense for any of the metadata fields to have empty string values.

Due to the use of empty strings, we would have to change the behaviour of utility.Set to not record empty strings. Because I have only modified metadata types, and not all model types, I instead changed the metadata types' Fields methods to stop using utility.Set and implemented a limited version of #3565 which is more explicit about omitting empty strings.

These changes yield a significant performance improvement in micro-benchmarks, both in decoding and transformation. Decoding improvements can be attributed to fewer allocations, while transformation improvements can be attributed to:

- fewer allocations
-- no interface allocations, or unnecessary deep copying of maps, due to utility.Set
-- lazy map construction
- less reflection, due to not using utility.Set
- less pointer indirection

name                   old time/op    new time/op    delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8    1.16µs ± 6%    0.38µs ±11%  -67.59%  (p=0.008 n=5+5)
MetadataSet/full-8       11.9µs ± 4%     5.3µs ± 6%  -55.53%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         9.70µs ± 1%    9.30µs ±17%     ~     (p=0.690 n=5+5)

name                   old alloc/op   new alloc/op   delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      896B ± 0%      368B ± 0%  -58.93%  (p=0.008 n=5+5)
MetadataSet/full-8       14.0kB ± 0%     6.2kB ± 0%  -55.36%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         1.31kB ± 0%    1.06kB ± 0%  -18.96%  (p=0.000 n=5+4)

name                   old allocs/op  new allocs/op  delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      10.0 ± 0%       4.0 ± 0%  -60.00%  (p=0.008 n=5+5)
MetadataSet/full-8          114 ± 0%        68 ± 0%  -40.35%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8           61.0 ± 0%      28.0 ± 0%  -54.10%  (p=0.008 n=5+5)

* model/modeldecoder: benchmark DecodeMetadata

* Benchmark recycled memory decoding

* model/modeldecoder: update decoding

* model/metadata: use non-pointer fields

* Adapt inputs to model changes

* model/metadata: benchmark Metadata.Set

* model: fix golint error (Id->ID)

beater/api/profile/handler_test.go

@@ -148,7 +148,7 @@ func TestHandler(t *testing.T) {
 					require.Len(t, req.Transformables, 2)
 					for _, tr := range req.Transformables {
 						profile := tr.(profile.PprofProfile)
-						assert.Equal(t, "foo", *profile.Metadata.Service.Name)
+						assert.Equal(t, "foo", profile.Metadata.Service.Name)
 					}
 					return nil
 				}

model/context.go

@@ -21,10 +21,9 @@ import (
 	"net"
 	"net/http"
 
-	"github.com/elastic/apm-server/model/metadata"
-
 	"github.com/elastic/beats/v7/libbeat/common"
 
+	"github.com/elastic/apm-server/model/metadata"
 	"github.com/elastic/apm-server/utility"
 )
 
@@ -68,6 +67,9 @@ type Page struct {
 }
 
 // Labels holds user defined information nested under key tags
+//
+// TODO(axw) either get rid of this type, or use it consistently
+// in all model types (looking at you, Metadata).
 type Labels common.MapStr
 
 // Custom holds user defined information nested under key custom

model/error/event.go

@@ -231,7 +231,7 @@ func (e *Event) addException(ctx context.Context, tctx *transform.Context, chain
 
 		// TODO(axw) we should be using a merged service object, combining
 		// the stream metadata and event-specific service info.
-		st := exception.Stacktrace.Transform(ctx, tctx, e.Metadata.Service)
+		st := exception.Stacktrace.Transform(ctx, tctx, &e.Metadata.Service)
 		utility.Set(ex, "stacktrace", st)
 
 		result = append(result, ex)
@@ -251,7 +251,7 @@ func (e *Event) addLog(ctx context.Context, tctx *transform.Context) {
 	utility.Set(log, "level", e.Log.Level)
 	// TODO(axw) we should be using a merged service object, combining
 	// the stream metadata and event-specific service info.
-	st := e.Log.Stacktrace.Transform(ctx, tctx, e.Metadata.Service)
+	st := e.Log.Stacktrace.Transform(ctx, tctx, &e.Metadata.Service)
 	utility.Set(log, "stacktrace", st)
 
 	e.add("log", log)

model/error/event_test.go

@@ -223,9 +223,9 @@ func TestEventFields(t *testing.T) {
 
 				// Service name and version are required for sourcemapping.
 				Metadata: metadata.Metadata{
-					Service: &metadata.Service{
-						Name:    tests.StringPtr("myservice"),
-						Version: tests.StringPtr("myservice"),
+					Service: metadata.Service{
+						Name:    "myservice",
+						Version: "myservice",
 					},
 				},
 			},
@@ -291,11 +291,11 @@ func TestEvents(t *testing.T) {
 
 	serviceName, agentName, version := "myservice", "go", "1.0"
 	md := metadata.Metadata{
-		Service: &metadata.Service{
-			Name: &serviceName, Version: &version,
-			Agent: metadata.Agent{Name: &agentName, Version: &version},
+		Service: metadata.Service{
+			Name: serviceName, Version: version,
+			Agent: metadata.Agent{Name: agentName, Version: version},
 		},
-		User:   &metadata.User{Id: &uid},
+		User:   metadata.User{ID: uid},
 		Labels: common.MapStr{"label": 101},
 	}
 
@@ -359,7 +359,7 @@ func TestEvents(t *testing.T) {
 				},
 				TransactionId:      &trId,
 				TransactionSampled: &sampledTrue,
-				User:               &metadata.User{Email: &email, IP: net.ParseIP(userIp), UserAgent: &userAgent},
+				User:               &metadata.User{Email: email, IP: net.ParseIP(userIp), UserAgent: userAgent},
 				Labels:             &labels,
 				Page:               &m.Page{Url: &url, Referer: &referer},
 				Custom:             &custom,
@@ -402,7 +402,7 @@ func TestEvents(t *testing.T) {
 			Transformable: &Event{
 				Timestamp: timestamp,
 				Metadata:  md,
-				Service:   &metadata.Service{Version: &serviceVersion},
+				Service:   &metadata.Service{Version: serviceVersion},
 			},
 			Output: common.MapStr{
 				"service":   common.MapStr{"name": serviceName, "version": serviceVersion},
@@ -755,9 +755,9 @@ func md5With(args ...string) []byte {
 func TestSourcemapping(t *testing.T) {
 	event := Event{
 		Metadata: metadata.Metadata{
-			Service: &metadata.Service{
-				Name:    tests.StringPtr("foo"),
-				Version: tests.StringPtr("bar"),
+			Service: metadata.Service{
+				Name:    "foo",
+				Version: "bar",
 			},
 		},
 		Exception: &Exception{

model/metadata/container.go

@@ -18,7 +18,6 @@
 package metadata
 
 import (
-	"github.com/elastic/apm-server/utility"
 	"github.com/elastic/beats/v7/libbeat/common"
 )
 
@@ -30,7 +29,7 @@ func (k *Container) fields() common.MapStr {
 	if k == nil {
 		return nil
 	}
-	container := common.MapStr{}
-	utility.Set(container, "id", k.ID)
-	return container
+	var container mapStr
+	container.maybeSetString("id", k.ID)
+	return common.MapStr(container)
 }

model/metadata/container_test.go

@@ -34,15 +34,11 @@ func TestContainerTransform(t *testing.T) {
 	}{
 		{
 			Container: Container{},
-			Output:    common.MapStr{"id": ""},
+			Output:    nil,
 		},
 		{
-			Container: Container{
-				ID: id,
-			},
-			Output: common.MapStr{
-				"id": id,
-			},
+			Container: Container{ID: id},
+			Output:    common.MapStr{"id": id},
 		},
 	}
 

model/metadata/kubernetes.go

@@ -18,33 +18,31 @@
 package metadata
 
 import (
-	"github.com/elastic/apm-server/utility"
 	"github.com/elastic/beats/v7/libbeat/common"
 )
 
 type Kubernetes struct {
-	Namespace *string
-	NodeName  *string
-	PodName   *string
-	PodUID    *string
+	Namespace string
+	NodeName  string
+	PodName   string
+	PodUID    string
 }
 
 func (k *Kubernetes) fields() common.MapStr {
-	if k == nil {
-		return nil
-	}
-	kubernetes := common.MapStr{}
-	utility.Set(kubernetes, "namespace", k.Namespace)
-
-	node := common.MapStr{}
-	utility.Set(node, "name", k.NodeName)
+	var kubernetes mapStr
+	kubernetes.maybeSetString("namespace", k.Namespace)
 
-	pod := common.MapStr{}
-	utility.Set(pod, "name", k.PodName)
-	utility.Set(pod, "uid", k.PodUID)
+	var node mapStr
+	if node.maybeSetString("name", k.NodeName) {
+		kubernetes.set("node", common.MapStr(node))
+	}
 
-	utility.Set(kubernetes, "node", node)
-	utility.Set(kubernetes, "pod", pod)
+	var pod mapStr
+	pod.maybeSetString("name", k.PodName)
+	pod.maybeSetString("uid", k.PodUID)
+	if pod != nil {
+		kubernetes.set("pod", common.MapStr(pod))
+	}
 
-	return kubernetes
+	return common.MapStr(kubernetes)
 }

model/metadata/kubernetes_test.go

@@ -34,14 +34,14 @@ func TestKubernetesTransform(t *testing.T) {
 	}{
 		{
 			Kubernetes: Kubernetes{},
-			Output:     common.MapStr{},
+			Output:     nil,
 		},
 		{
 			Kubernetes: Kubernetes{
-				Namespace: &namespace,
-				NodeName:  &nodename,
-				PodName:   &podname,
-				PodUID:    &poduid,
+				Namespace: namespace,
+				NodeName:  nodename,
+				PodName:   podname,
+				PodUID:    poduid,
 			},
 			Output: common.MapStr{
 				"namespace": namespace,

model/metadata/mapstr.go

@@ -0,0 +1,45 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package metadata
+
+import "github.com/elastic/beats/v7/libbeat/common"
+
+type mapStr common.MapStr
+
+func (m *mapStr) set(k string, v interface{}) {
+	if *m == nil {
+		*m = make(mapStr)
+	}
+	(*m)[k] = v
+}
+
+func (m *mapStr) maybeSetString(k, v string) bool {
+	if v != "" {
+		m.set(k, v)
+		return true
+	}
+	return false
+}
+
+func (m *mapStr) maybeSetMapStr(k string, v common.MapStr) bool {
+	if len(v) > 0 {
+		m.set(k, v)
+		return true
+	}
+	return false
+}

model/metadata/metadata.go

@@ -18,39 +18,39 @@
 package metadata
 
 import (
-	"github.com/elastic/beats/v7/libbeat/common"
-
 	"github.com/elastic/apm-server/utility"
+	"github.com/elastic/beats/v7/libbeat/common"
 )
 
 type Metadata struct {
-	Service *Service
-	Process *Process
-	System  *System
-	User    *User
+	Service Service
+	Process Process
+	System  System
+	User    User
 	Labels  common.MapStr
 }
 
-func (m *Metadata) Set(fields common.MapStr) common.MapStr {
-	containerFields := m.System.containerFields()
-	hostFields := m.System.fields()
-	utility.Set(fields, "service", m.Service.Fields(get(containerFields, "id"), get(hostFields, "name")))
-	utility.Set(fields, "agent", m.Service.AgentFields())
-	utility.Set(fields, "host", hostFields)
-	utility.Set(fields, "process", m.Process.fields())
-	utility.Set(fields, "user", m.User.Fields())
-	utility.Set(fields, "client", m.User.ClientFields())
-	utility.Set(fields, "user_agent", m.User.UserAgentFields())
-	utility.Set(fields, "container", containerFields)
-	utility.Set(fields, "kubernetes", m.System.kubernetesFields())
-	// to be merged with specific event labels, these should be overwritten in case of conflict
-	utility.Set(fields, "labels", m.Labels)
-	return fields
-}
-
-func get(m common.MapStr, key string) string {
-	if val, ok := m[key].(string); ok {
-		return val
+func (m *Metadata) Set(out common.MapStr) common.MapStr {
+	fields := (*mapStr)(&out)
+	fields.maybeSetMapStr("service", m.Service.Fields(m.System.Container.ID, m.System.name()))
+	fields.maybeSetMapStr("agent", m.Service.AgentFields())
+	fields.maybeSetMapStr("host", m.System.fields())
+	fields.maybeSetMapStr("process", m.Process.fields())
+	fields.maybeSetMapStr("user", m.User.Fields())
+	fields.maybeSetMapStr("client", m.User.ClientFields())
+	fields.maybeSetMapStr("user_agent", m.User.UserAgentFields())
+	fields.maybeSetMapStr("container", m.System.containerFields())
+	fields.maybeSetMapStr("kubernetes", m.System.kubernetesFields())
+	if len(m.Labels) > 0 {
+		// These labels are merged with event-specific labels,
+		// hence we clone the map to avoid updating the shared
+		// metadata map.
+		//
+		// TODO(axw) we should only clone as needed or, better,
+		// avoid cloning altogether. For example, we could use
+		// DeepUpdateNoOverwrite in the other direction to copy
+		// the shared map into an event-specific labels map.
+		utility.Set(out, "labels", m.Labels)
 	}
-	return ""
+	return out
 }