expected_metrics follow-up

integration_test/cmd/generate_expected_metrics/generate_expected_metrics.go

@@ -32,9 +32,7 @@ package main
 
 import (
 	"context"
-	"errors"
 	"fmt"
-	"io/fs"
 	"log"
 	"os"
 	"path"
@@ -64,7 +62,7 @@ var (
 	}
 )
 
-type expectedMetricsMap map[string]*common.ExpectedMetric
+type expectedMetricsMap map[string]common.ExpectedMetric
 
 func main() {
 	if err := run(); err != nil {
@@ -91,33 +89,29 @@ func run() error {
 		// For each new metric, either update the corresponding existing metric,
 		// or add it.
 		for _, newMetric := range newMetrics {
-			if existingMetric, ok := existingMetrics[newMetric.Type]; ok {
-				updateMetric(existingMetric, newMetric)
-			} else {
-				existingMetrics[newMetric.Type] = newMetric
-			}
+			existingMetrics[newMetric.Type] = updateMetric(existingMetrics[newMetric.Type], newMetric)
 		}
 		err = multierr.Append(err, writeExpectedMetrics(app, existingMetrics))
 	}
 	return err
 }
 
 // listMetrics calls projects.metricDescriptors.list with the given project ID and filter.
-func listMetrics(ctx context.Context, project string, filter string) ([]*metric.MetricDescriptor, error) {
+func listMetrics(ctx context.Context, project string, filter string) ([]metric.MetricDescriptor, error) {
 	req := &monitoringpb.ListMetricDescriptorsRequest{
 		Name:   "projects/" + project + "/metricDescriptors/",
 		Filter: filter,
 	}
 	it := monClient.ListMetricDescriptors(ctx, req)
-	metrics := make([]*metric.MetricDescriptor, 0)
+	metrics := make([]metric.MetricDescriptor, 0)
 	for {
 		m, err := it.Next()
 		if err == iterator.Done {
 			break
 		} else if err != nil {
 			return nil, err
 		}
-		metrics = append(metrics, m)
+		metrics = append(metrics, *m)
 	}
 	return metrics, nil
 }
@@ -172,12 +166,12 @@ func getAppName(metricType string) string {
 }
 
 // toExpectedMetric converts from metric.MetricDescriptor to ExpectedMetric.
-func toExpectedMetric(metric *metric.MetricDescriptor) *common.ExpectedMetric {
+func toExpectedMetric(metric metric.MetricDescriptor) common.ExpectedMetric {
 	labels := make(map[string]string)
 	for _, l := range metric.Labels {
 		labels[l.Key] = ".*"
 	}
-	return &common.ExpectedMetric{
+	return common.ExpectedMetric{
 		Type:              metric.Type,
 		Kind:              metric.MetricKind.String(),
 		ValueType:         metric.ValueType.String(),
@@ -186,83 +180,98 @@ func toExpectedMetric(metric *metric.MetricDescriptor) *common.ExpectedMetric {
 	}
 }
 
-func expectedMetricsFilename(app string) string {
-	return path.Join(scriptsDir, "applications", app, "expected_metrics.yaml")
+func metadataFilename(app string) string {
+	return path.Join(scriptsDir, "applications", app, "metadata.yaml")
+}
+
+func readMetadata(app string) (common.IntegrationMetadata, error) {
+	file := metadataFilename(app)
+	serialized, err := os.ReadFile(file)
+	var metadata common.IntegrationMetadata
+	if err != nil {
+		return metadata, err
+	}
+	err = yaml.Unmarshal(serialized, &metadata)
+	return metadata, err
 }
 
-// readExpectedMetrics reads in the existing expected_metrics.yaml
-// file for the given app as a map keyed on metric type. If no file
-// exists, an empty map is returned.
+// readExpectedMetrics reads in metrics from the existing metadata.yaml
+// file for the given app as a map keyed on metric type. If no metrics
+// exist, an empty map is returned.
 // Otherwise, its contents are returned, or an error if it could
 // not be unmarshaled.
 func readExpectedMetrics(app string) (expectedMetricsMap, error) {
-	file := expectedMetricsFilename(app)
-	serialized, err := os.ReadFile(file)
-	metricsByType := make(expectedMetricsMap)
-	if errors.Is(err, fs.ErrNotExist) {
-		return metricsByType, nil
-	} else if err != nil {
-		return nil, err
-	}
-	var metrics []common.ExpectedMetric
-	if err = yaml.Unmarshal(serialized, &metrics); err != nil {
+	metadata, err := readMetadata(app)
+	if err != nil {
 		return nil, err
 	}
-	for _, m := range metrics {
+	metricsByType := make(expectedMetricsMap)
+	expectedMetrics := metadata.ExpectedMetrics
+	for _, m := range expectedMetrics {
 		m := m
 		if _, ok := metricsByType[m.Type]; ok {
-			return nil, fmt.Errorf("duplicate metric type in %s/expected_metrics.yaml: %s", app, m.Type)
+			return nil, fmt.Errorf("duplicate expected_metrics type in %s/metadata.yaml: %s", app, m.Type)
 		}
-		metricsByType[m.Type] = &m
+		metricsByType[m.Type] = m
 	}
 	return metricsByType, nil
 }
 
 // writeExpectedMetrics writes the given map's values as a slice
-// to the expected_metrics.yaml associated with the given app. Metrics
+// to the metadata.yaml associated with the given app. Metrics
 // are written in alphabetical order by type.
 func writeExpectedMetrics(app string, metrics expectedMetricsMap) error {
-	metricsSlice := make([]common.ExpectedMetric, 0)
+	metadata, err := readMetadata(app)
+	if err != nil {
+		return err
+	}
+	expectedMetrics := make([]common.ExpectedMetric, 0)
 	for _, m := range metrics {
-		metricsSlice = append(metricsSlice, *m)
+		expectedMetrics = append(expectedMetrics, m)
 	}
-	sort.Slice(metricsSlice, func(i, j int) bool { return metricsSlice[i].Type < metricsSlice[j].Type })
-	serialized, err := yaml.Marshal(metricsSlice)
+	sort.Slice(expectedMetrics, func(i, j int) bool { return expectedMetrics[i].Type < expectedMetrics[j].Type })
+	metadata.ExpectedMetrics = expectedMetrics
+	serialized, err := yaml.Marshal(metadata)
 	if err != nil {
 		return err
 	}
-	file := expectedMetricsFilename(app)
-	return os.WriteFile(file, serialized, 0644)
+	return os.WriteFile(metadataFilename(app), serialized, 0644)
 }
 
-// updateMetric updates the given metric in-place using values from withValuesFrom.
+// updateMetric returns the given metric with updates applied from withValuesFrom.
 // Existing Optional and Representative values are preserved, as well as existing
 // label patterns. All other values are copied from withValuesFrom. Existing label
 // keys not present in withValuesFrom.Labels are dropped.
-func updateMetric(toUpdate *common.ExpectedMetric, withValuesFrom *common.ExpectedMetric) {
+// If toUpdate.Type is empty, then withValuesFrom is returned.
+func updateMetric(toUpdate common.ExpectedMetric, withValuesFrom common.ExpectedMetric) common.ExpectedMetric {
+	if toUpdate.Type == "" {
+		// Empty struct to update; just copy over the new one
+		return withValuesFrom
+	}
 	if toUpdate.Type != withValuesFrom.Type {
 		panic(fmt.Errorf("updateMetric: attempted to update metric with mismatched type: %s, %s", toUpdate.Type, withValuesFrom.Type))
 	}
-	toUpdate.Kind = withValuesFrom.Kind
-	toUpdate.ValueType = withValuesFrom.ValueType
-	toUpdate.MonitoredResource = withValuesFrom.MonitoredResource
+	result := toUpdate
+	result.Kind = withValuesFrom.Kind
+	result.ValueType = withValuesFrom.ValueType
+	result.MonitoredResource = withValuesFrom.MonitoredResource
+	result.Labels = make(map[string]string)
 
 	// TODO: Refactor to a simple map copy once we improve listMetrics to fetch
 	// label patterns automatically.
 
-	// Copy new label keys
+	// The keys of result.Labels should be the same as withValuesFrom.Labels,
+	// except that existing values/patterns are preserved.
 	for k, v := range withValuesFrom.Labels {
-		// Don't overwrite existing patterns
-		if _, ok := toUpdate.Labels[k]; !ok {
-			toUpdate.Labels[k] = v
-		}
-	}
-	// Remove dropped label keys
-	for k := range toUpdate.Labels {
-		if _, ok := withValuesFrom.Labels[k]; !ok {
-			delete(toUpdate.Labels, k)
+		existingPattern, ok := toUpdate.Labels[k]
+		if ok {
+			result.Labels[k] = existingPattern
+		} else {
+			result.Labels[k] = v
 		}
 	}
+
+	return result
 }
 
 func init() {

integration_test/common/common.go

@@ -18,8 +18,8 @@ package common
 
 import (
 	"fmt"
-	"strings"
 
+	"github.com/go-playground/validator/v10"
 	"go.uber.org/multierr"
 )
 
@@ -29,55 +29,71 @@ type ExpectedMetric struct {
 	// The metric type, for example workload.googleapis.com/apache.current_connections.
 	Type string `yaml:"type"`
 	// The value type, for example INT64.
-	ValueType string `yaml:"value_type"`
+	ValueType string `yaml:"value_type" validate:"oneof=BOOL INT64 DOUBLE STRING DISTRIBUTION"`
 	// The kind, for example GAUGE.
-	Kind string `yaml:"kind"`
+	Kind string `yaml:"kind" validate:"oneof=GAUGE DELTA CUMULATIVE"`
 	// The monitored resource, for example gce_instance.
-	// Currently we only test with gce_instance, so we expect
-	// all expectedMetricsEntries to have gce_instance.
-	MonitoredResource string `yaml:"monitored_resource"`
+	// Currently we only test with gce_instance.
+	MonitoredResource string `yaml:"monitored_resource" validate:"oneof=gce_instance"`
 	// Mapping of expected label keys to value patterns.
 	// Patterns are RE2 regular expressions.
 	Labels map[string]string `yaml:"labels"`
 	// If Optional is true, the test for this metric will be skipped.
-	Optional bool `yaml:"optional,omitempty"`
+	Optional bool `yaml:"optional,omitempty" validate:"excluded_with=Representative"`
 	// Exactly one metric in each expected_metrics.yaml must
 	// have Representative set to true. This metric can be used
 	// to test that the integration is enabled.
-	Representative bool `yaml:"representative,omitempty"`
+	Representative bool `yaml:"representative,omitempty" validate:"excluded_with=Optional"`
 }
 
+type LogFields struct {
+	Name        string `yaml:"name" validate:"required"`
+	ValueRegex  string `yaml:"value_regex"`
+	Type        string `yaml:"type" validate:"required"`
+	Description string `yaml:"description" validate:"required"`
+}
+
+type ExpectedLog struct {
+	LogName string      `yaml:"log_name" validate:"required"`
+	Fields  []LogFields `yaml:"fields" validate:"required"`
+}
+
+type MinimumSupportedAgentVersion struct {
+	Logging string `yaml:"logging"`
+	Metrics string `yaml:"metrics"`
+}
+
+type IntegrationMetadata struct {
+	PublicUrl                    string                       `yaml:"public_url"`
+	ShortName                    string                       `yaml:"short_name" validate:"required"`
+	LongName                     string                       `yaml:"long_name" validate:"required"`
+	ConfigureIntegration         string                       `yaml:"configure_integration"`
+	ExpectedLogs                 []ExpectedLog                `yaml:"expected_logs"`
+	ExpectedMetrics              []ExpectedMetric             `yaml:"expected_metrics"`
+	MinimumSupportedAgentVersion MinimumSupportedAgentVersion `yaml:"minimum_supported_agent_version"`
+	SupportedAppVersion          []string                     `yaml:"supported_app_version" validate:"required"`
+}
+
+var validate *validator.Validate
+
 // ValidateMetrics checks that all enum fields have valid values and that
 // there is exactly one representative metric in the slice.
 func ValidateMetrics(metrics []ExpectedMetric) error {
 	var err error
-	// Field validation
-	expectedKinds := []string{"GAUGE", "DELTA", "CUMULATIVE"}
-	expectedValueTypes := []string{"BOOL", "INT64", "DOUBLE", "STRING", "DISTRIBUTION"}
-	expectedResource := "gce_instance"
-	for _, metric := range metrics {
-		innerErrs := make([]string, 0)
-		if !SliceContains(expectedKinds, metric.Kind) {
-			innerErrs = append(innerErrs, fmt.Sprintf("invalid kind %s (must be %v)", metric.Kind, expectedKinds))
-		}
-		if !SliceContains(expectedValueTypes, metric.ValueType) {
-			innerErrs = append(innerErrs, fmt.Sprintf("invalid value_type %s (must be %v)", metric.ValueType, expectedValueTypes))
-		}
-		if expectedResource != metric.MonitoredResource {
-			innerErrs = append(innerErrs, fmt.Sprintf("invalid monitored_resource %s (must be %v)", metric.MonitoredResource, expectedResource))
-		}
-		if len(innerErrs) > 0 {
-			err = multierr.Append(err, fmt.Errorf("%s: %v", metric.Type, strings.Join(innerErrs, ", ")))
+
+	// Field validation, one-by-one for better error messages
+	for _, m := range metrics {
+		vErr := validate.Struct(m)
+		if vErr != nil {
+			err = multierr.Append(err, fmt.Errorf("%s: %v", m.Type, vErr))
 		}
 	}
+
 	// Representative validation
 	representativeCount := 0
-	for _, metric := range metrics {
-		if metric.Representative {
+	for _, m := range metrics {
+		if m.Representative {
 			representativeCount += 1
-			if metric.Optional {
-				err = multierr.Append(err, fmt.Errorf("%s: metric cannot be both representative and optional", metric.Type))
-			}
 		}
 	}
 	if representativeCount != 1 {
@@ -94,3 +110,7 @@ func SliceContains(slice []string, toFind string) bool {
 	}
 	return false
 }
+
+func init() {
+	validate = validator.New()
+}

integration_test/third_party_apps_test.go

@@ -180,38 +180,10 @@ func installAgent(ctx context.Context, logger *logging.DirectoryLogger, vm *gce.
 	return nonRetryable, agents.InstallPackageFromGCS(ctx, logger, vm, packagesInGCS)
 }
 
-type logFields struct {
-	Name        string `yaml:"name" validate:"required"`
-	ValueRegex  string `yaml:"value_regex"`
-	Type        string `yaml:"type" validate:"required"`
-	Description string `yaml:"description" validate:"required"`
-}
-
-type expectedLog struct {
-	LogName string      `yaml:"log_name" validate:"required"`
-	Fields  []logFields `yaml:"fields" validate:"required"`
-}
-
-type minimumSupportedAgentVersion struct {
-	Logging string `yaml:"logging"`
-	Metrics string `yaml:"metrics"`
-}
-
-type integrationMetadata struct {
-	PublicUrl                    string                       `yaml:"public_url"`
-	ShortName                    string                       `yaml:"short_name" validate:"required"`
-	LongName                     string                       `yaml:"long_name" validate:"required"`
-	ConfigureIntegration         string                       `yaml:"configure_integration"`
-	ExpectedLogs                 []expectedLog                `yaml:"expected_logs"`
-	ExpectedMetrics              []common.ExpectedMetric      `yaml:"expected_metrics"`
-	MinimumSupportedAgentVersion minimumSupportedAgentVersion `yaml:"minimum_supported_agent_version"`
-	SupportedAppVersion          []string                     `yaml:"supported_app_version" validate:"required"`
-}
-
 // constructQuery converts the given struct of:
 //   field name => field value regex
 // into a query filter to pass to the logging API.
-func constructQuery(fields []logFields) string {
+func constructQuery(fields []common.LogFields) string {
 	var parts []string
 	for _, field := range fields {
 		if field.ValueRegex != "" {
@@ -221,7 +193,7 @@ func constructQuery(fields []logFields) string {
 	return strings.Join(parts, " AND ")
 }
 
-func runLoggingTestCases(ctx context.Context, logger *logging.DirectoryLogger, vm *gce.VM, logs []expectedLog) error {
+func runLoggingTestCases(ctx context.Context, logger *logging.DirectoryLogger, vm *gce.VM, logs []common.ExpectedLog) error {
 
 	// Wait for each entry in LogEntries concurrently. This is especially helpful
 	// when	the assertions fail: we don't want to wait for each one to time out
@@ -415,7 +387,7 @@ func runSingleTest(ctx context.Context, logger *logging.DirectoryLogger, vm *gce
 	// Check if metadata.yaml exists, and run the test cases if it does.
 	if testCaseBytes, err := readFileFromScriptsDir(path.Join("applications", app, "metadata.yaml")); err == nil {
 		logger.ToMainLog().Println("found metadata.yaml, parsing...")
-		var metadata integrationMetadata
+		var metadata common.IntegrationMetadata
 		err := yaml.UnmarshalStrict(testCaseBytes, &metadata)
 		if err != nil {
 			return nonRetryable, fmt.Errorf("could not unmarshal contents of metadata.yaml: %v", err)