[Heartbeat] Use service.name not service_name in configs (elastic#20330)

andrewvc · andrewvc · commit 25c564a29d9a · 2020-11-12T14:22:10.000-06:00
Using the nesting this way lets us handle future fields like service.environment more cleanly if/when they become available. See https://github.com/elastic/ecs/blob/master/rfcs/text/0002-rfc-environment.md for the proposal to add service.environment This is a follow-up to elastic#19932 which has not yet been released, so this is not a breaking change. (cherry picked from commit 425271b)
diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc
@@ -72,7 +72,6 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 
 *Heartbeat*
 
-
 *Journalbeat*
 
 - Improve parsing of syslog.pid in journalbeat to strip the username when present {pull}16116[16116]
@@ -242,6 +241,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 
 *Heartbeat*
 
+- The `service_name` monitor option is being replaced with `service.name` which is more correct. We will support the old option till 8.0. {pull}20330[20330]
 
 *Journalbeat*
 
diff --git a/heartbeat/_meta/config/beat.reference.yml.tmpl b/heartbeat/_meta/config/beat.reference.yml.tmpl
@@ -32,7 +32,7 @@ heartbeat.monitors:
   name: my-icmp-monitor
 
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  # service_name: my-apm-service-name
+  # service.name: my-apm-service-name
 
   # Enable/Disable monitor
   #enabled: true
diff --git a/heartbeat/_meta/config/beat.yml.tmpl b/heartbeat/_meta/config/beat.yml.tmpl
@@ -33,7 +33,7 @@ heartbeat.monitors:
   # Total test connection and data exchange timeout
   #timeout: 16s
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  #service_name: my-apm-service-name
+  #service.name: my-apm-service-name
 
 {{header "Elasticsearch template setting"}}
 
diff --git a/heartbeat/beater/heartbeat.go b/heartbeat/beater/heartbeat.go
@@ -54,7 +54,6 @@ func New(b *beat.Beat, rawConfig *common.Config) (beat.Beater, error) {
 	if err := rawConfig.Unpack(&parsedConfig); err != nil {
 		return nil, fmt.Errorf("Error reading config file: %v", err)
 	}
-
 	limit := parsedConfig.Scheduler.Limit
 	locationName := parsedConfig.Scheduler.Location
 	if locationName == "" {
diff --git a/heartbeat/docs/getting-started.asciidoc b/heartbeat/docs/getting-started.asciidoc
@@ -81,7 +81,7 @@ heartbeat.monitors:
 - type: http
   schedule: '@every 5s'
   urls: ["http://example.net"]
-  service_name: apm-service-name <4>
+  service.name: apm-service-name <4>
   id: my-http-service
   name: My HTTP Service
 ----------------------------------------------------------------------
@@ -92,7 +92,7 @@ https://github.com/gorhill/cronexpr#implementation[this `cronexpr` implementatio
 was started. Heartbeat adds the `@every` keyword to the syntax provided by the
 `cronexpr` package.
 <3> The `mode` specifies whether to ping one IP (`any`) or all resolvable IPs
-<4> The `service_name` field can be used to integrate heartbeat with elastic APM via the Uptime UI.
+<4> The `service.name` field can be used to integrate heartbeat with elastic APM via the Uptime UI.
 
 include::{libbeat-dir}/shared/config-check.asciidoc[]
 
diff --git a/heartbeat/docs/heartbeat-options.asciidoc b/heartbeat/docs/heartbeat-options.asciidoc
@@ -38,7 +38,7 @@ heartbeat.monitors:
 - type: http
   id: service-status
   name: Service Status
-  service_name: my-apm-service-name
+  service.name: my-apm-service-name
   hosts: ["http://localhost:80/service/status"]
   check.response.status: [200]
   schedule: '@every 5s'
diff --git a/heartbeat/docs/monitors/monitor-common-options.asciidoc b/heartbeat/docs/monitors/monitor-common-options.asciidoc
@@ -35,7 +35,7 @@ as `monitor.name`.
 
 [float]
 [[service-name]]
-==== `service_name`
+==== `service.name`
 
 Optional APM service name for this monitor. Corresponds to the `service.name` ECS field. Set this when monitoring an app
 that is also using APM to enable integrations between Uptime and APM data in Kibana.
diff --git a/heartbeat/heartbeat.reference.yml b/heartbeat/heartbeat.reference.yml
@@ -32,7 +32,7 @@ heartbeat.monitors:
   name: my-icmp-monitor
 
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  # service_name: my-apm-service-name
+  # service.name: my-apm-service-name
 
   # Enable/Disable monitor
   #enabled: true
diff --git a/heartbeat/heartbeat.yml b/heartbeat/heartbeat.yml
@@ -33,7 +33,7 @@ heartbeat.monitors:
   # Total test connection and data exchange timeout
   #timeout: 16s
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  #service_name: my-apm-service-name
+  #service.name: my-apm-service-name
 
 # ======================= Elasticsearch template setting =======================
 
diff --git a/heartbeat/monitors.d/sample.http.yml.disabled b/heartbeat/monitors.d/sample.http.yml.disabled
@@ -11,7 +11,7 @@
   name: My HTTP Monitor
 
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  #service_name: my-apm-service-name
+  #service.name: my-apm-service-name
 
   # Enable/Disable monitor
   #enabled: true
diff --git a/heartbeat/monitors.d/sample.icmp.yml.disabled b/heartbeat/monitors.d/sample.icmp.yml.disabled
@@ -11,7 +11,7 @@
   name: My ICMP Monitor
 
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  #service_name: my-apm-service-name
+  #service.name: my-apm-service-name
 
   # Enable/Disable monitor
   #enabled: true
diff --git a/heartbeat/monitors.d/sample.tcp.yml.disabled b/heartbeat/monitors.d/sample.tcp.yml.disabled
@@ -13,7 +13,7 @@
   name: My TCP monitor
 
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  #service_name: my-apm-service-name
+  #service.name: my-apm-service-name
 
   # Enable/Disable monitor
   #enabled: true
diff --git a/heartbeat/monitors/stdfields/stdfields.go b/heartbeat/monitors/stdfields/stdfields.go
@@ -18,6 +18,7 @@
 package stdfields
 
 import (
+	"fmt"
 	"time"
 
 	"github.com/pkg/errors"
@@ -29,24 +30,38 @@ import (
 // ErrPluginDisabled is returned when the monitor plugin is marked as disabled.
 var ErrPluginDisabled = errors.New("Monitor not loaded, plugin is disabled")
 
+type ServiceFields struct {
+	Name string `config:"name"`
+}
+
 // StdMonitorFields represents the generic configuration options around a monitor plugin.
 type StdMonitorFields struct {
-	ID          string             `config:"id"`
-	Name        string             `config:"name"`
-	Type        string             `config:"type" validate:"required"`
-	Schedule    *schedule.Schedule `config:"schedule" validate:"required"`
-	Timeout     time.Duration      `config:"timeout"`
-	ServiceName string             `config:"service_name"`
-	Enabled     bool               `config:"enabled"`
+	ID                string             `config:"id"`
+	Name              string             `config:"name"`
+	Type              string             `config:"type" validate:"required"`
+	Schedule          *schedule.Schedule `config:"schedule" validate:"required"`
+	Timeout           time.Duration      `config:"timeout"`
+	Service           ServiceFields      `config:"service"`
+	LegacyServiceName string             `config:"service_name"`
+	Enabled           bool               `config:"enabled"`
 }
 
 func ConfigToStdMonitorFields(config *common.Config) (StdMonitorFields, error) {
 	mpi := StdMonitorFields{Enabled: true}
 
 	if err := config.Unpack(&mpi); err != nil {
+		fmt.Printf("HIER %s", err)
 		return mpi, errors.Wrap(err, "error unpacking monitor plugin config")
 	}
 
+	// Use `service_name` if `service.name` is unspecified
+	// `service_name` was only document in the 7.10.0 release.
+	if mpi.LegacyServiceName != "" {
+		if mpi.Service.Name == "" {
+			mpi.Service.Name = mpi.LegacyServiceName
+		}
+	}
+
 	if !mpi.Enabled {
 		return mpi, ErrPluginDisabled
 	}
diff --git a/heartbeat/monitors/stdfields/stdfields_test.go b/heartbeat/monitors/stdfields/stdfields_test.go
@@ -0,0 +1,65 @@
+// 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 stdfields
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/elastic/beats/v7/libbeat/common"
+)
+
+func TestLegacyServiceNameConfig(t *testing.T) {
+	srvName := "myService"
+
+	configBase := func() common.MapStr {
+		return common.MapStr{
+			"type":     "http",
+			"id":       "myId",
+			"schedule": "@every 1s",
+		}
+	}
+
+	legacyOnly := configBase()
+	legacyOnly["service_name"] = srvName
+
+	newOnly := configBase()
+	newOnly["service"] = common.MapStr{"name": srvName}
+
+	mix := configBase()
+	mix["service"] = common.MapStr{"name": srvName}
+	mix["service_name"] = "ignoreMe"
+
+	confMaps := []common.MapStr{
+		legacyOnly,
+		newOnly,
+		mix,
+	}
+
+	for _, cm := range confMaps {
+		t.Run(fmt.Sprintf("given config map %#v", cm), func(t *testing.T) {
+			c, err := common.NewConfigFrom(cm)
+			require.NoError(t, err)
+			f, err := ConfigToStdMonitorFields(c)
+			require.Equal(t, srvName, f.Service.Name)
+		})
+	}
+
+}
diff --git a/heartbeat/monitors/wrappers/monitors.go b/heartbeat/monitors/wrappers/monitors.go
@@ -77,9 +77,9 @@ func addMonitorMeta(stdMonFields stdfields.StdMonitorFields, isMulti bool) jobs.
 				},
 			}
 
-			if stdMonFields.ServiceName != "" {
+			if stdMonFields.Service.Name != "" {
 				fieldsToMerge["service"] = common.MapStr{
-					"name": stdMonFields.ServiceName,
+					"name": stdMonFields.Service.Name,
 				}
 			}
 
diff --git a/heartbeat/monitors/wrappers/monitors_test.go b/heartbeat/monitors/wrappers/monitors_test.go
@@ -105,7 +105,7 @@ func TestSimpleJob(t *testing.T) {
 
 func TestJobWithServiceName(t *testing.T) {
 	fields := testMonFields
-	fields.ServiceName = "testServiceName"
+	fields.Service.Name = "testServiceName"
 	testCommonWrap(t, testDef{
 		"simple",
 		fields,
@@ -123,7 +123,7 @@ func TestJobWithServiceName(t *testing.T) {
 						"check_group": isdef.IsString,
 					},
 					"service": map[string]interface{}{
-						"name": fields.ServiceName,
+						"name": fields.Service.Name,
 					},
 				}),
 				hbtestllext.MonitorTimespanValidator,
diff --git a/x-pack/heartbeat/heartbeat.reference.yml b/x-pack/heartbeat/heartbeat.reference.yml
@@ -32,7 +32,7 @@ heartbeat.monitors:
   name: my-icmp-monitor
 
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  # service_name: my-apm-service-name
+  # service.name: my-apm-service-name
 
   # Enable/Disable monitor
   #enabled: true
diff --git a/x-pack/heartbeat/heartbeat.yml b/x-pack/heartbeat/heartbeat.yml
@@ -33,7 +33,7 @@ heartbeat.monitors:
   # Total test connection and data exchange timeout
   #timeout: 16s
   # Name of corresponding APM service, if Elastic APM is in use for the monitored service.
-  #service_name: my-apm-service-name
+  #service.name: my-apm-service-name
 
 # ======================= Elasticsearch template setting =======================
 

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,6 @@ func New(b beat.Beat, rawConfig common.Config) (beat.Beater, error) {`
`54`	`54`	`if err := rawConfig.Unpack(&parsedConfig); err != nil {`
`55`	`55`	`return nil, fmt.Errorf("Error reading config file: %v", err)`
`56`	`56`	`}`
`57`		`-`
`58`	`57`	`limit := parsedConfig.Scheduler.Limit`
`59`	`58`	`locationName := parsedConfig.Scheduler.Location`
`60`	`59`	`if locationName == "" {`