From 74729e731d3b795ab81f33875e62753cbba5d791 Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Mon, 16 Sep 2024 16:36:58 +0200 Subject: [PATCH] [mdatagen] Fix metric tests on boolean attributes (#11169) #### Description Since commit 8f3ca8a, `mdatagen` improperly generates metric tests: boolean attributes are always expected to be `true` in the output, no matter what the injected test value was. For example, [this line in collector-contrib](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/19b01db307d984e62f81f1880b63327ba092cd81/receiver/apachesparkreceiver/internal/metadata/generated_metrics_test.go#L1250) is incorrectly changed to expect `true` instead of `false` after updating mdatagen and running it again. The reason is [this line](https://github.com/open-telemetry/opentelemetry-collector/blob/8f3ca8aaf58539e5afab54d365f97624efe7cbc3/cmd/mdatagen/templates/metrics_test.go.tmpl#L182), which attempts to generate `assert.True` or `assert.False` based on the expected test value. However, `(attributeInfo $attr).TestValue` does not return a boolean but the *string* `true` or `false`, both of which are considered "truthy" by the templating engine. #### Testing I manually rebuilt mdatagen, ran `make generate` in collector-contrib, and ran the tests, to confirm that the test related to the previously mentioned change was no longer failing. --- .chloggen/fix-mdatagen-metrics-bool-attr.yaml | 25 +++++++++++++++++++ .../internal/samplereceiver/documentation.md | 1 + .../internal/metadata/generated_metrics.go | 7 +++--- .../metadata/generated_metrics_test.go | 5 +++- .../internal/samplereceiver/metadata.yaml | 16 ++++++++---- cmd/mdatagen/loader_test.go | 9 ++++++- cmd/mdatagen/templates/metrics_test.go.tmpl | 2 +- 7 files changed, 54 insertions(+), 11 deletions(-) create mode 100644 .chloggen/fix-mdatagen-metrics-bool-attr.yaml diff --git a/.chloggen/fix-mdatagen-metrics-bool-attr.yaml b/.chloggen/fix-mdatagen-metrics-bool-attr.yaml new file mode 100644 index 00000000000..5f78068bb71 --- /dev/null +++ b/.chloggen/fix-mdatagen-metrics-bool-attr.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: mdatagen + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fix incorrect generation of metric tests with boolean attributes. + +# One or more tracking issues or pull requests related to the change +issues: [11169] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/cmd/mdatagen/internal/samplereceiver/documentation.md b/cmd/mdatagen/internal/samplereceiver/documentation.md index 003a4cebbdb..f2370f811a5 100644 --- a/cmd/mdatagen/internal/samplereceiver/documentation.md +++ b/cmd/mdatagen/internal/samplereceiver/documentation.md @@ -84,6 +84,7 @@ metrics: | ---- | ----------- | ------ | | string_attr | Attribute with any string value. | Any Str | | boolean_attr | Attribute with a boolean value. | Any Bool | +| boolean_attr2 | Another attribute with a boolean value. | Any Bool | ### optional.metric.empty_unit diff --git a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go index 1e345a799b7..3baf1125eba 100644 --- a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go +++ b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics.go @@ -225,7 +225,7 @@ func (m *metricOptionalMetric) init() { m.data.Gauge().DataPoints().EnsureCapacity(m.capacity) } -func (m *metricOptionalMetric) recordDataPoint(start pcommon.Timestamp, ts pcommon.Timestamp, val float64, stringAttrAttributeValue string, booleanAttrAttributeValue bool) { +func (m *metricOptionalMetric) recordDataPoint(start pcommon.Timestamp, ts pcommon.Timestamp, val float64, stringAttrAttributeValue string, booleanAttrAttributeValue bool, booleanAttr2AttributeValue bool) { if !m.config.Enabled { return } @@ -235,6 +235,7 @@ func (m *metricOptionalMetric) recordDataPoint(start pcommon.Timestamp, ts pcomm dp.SetDoubleValue(val) dp.Attributes().PutStr("string_attr", stringAttrAttributeValue) dp.Attributes().PutBool("boolean_attr", booleanAttrAttributeValue) + dp.Attributes().PutBool("boolean_attr2", booleanAttr2AttributeValue) } // updateCapacity saves max length of data point slices that will be used for the slice capacity. @@ -559,8 +560,8 @@ func (mb *MetricsBuilder) RecordMetricInputTypeDataPoint(ts pcommon.Timestamp, i } // RecordOptionalMetricDataPoint adds a data point to optional.metric metric. -func (mb *MetricsBuilder) RecordOptionalMetricDataPoint(ts pcommon.Timestamp, val float64, stringAttrAttributeValue string, booleanAttrAttributeValue bool) { - mb.metricOptionalMetric.recordDataPoint(mb.startTime, ts, val, stringAttrAttributeValue, booleanAttrAttributeValue) +func (mb *MetricsBuilder) RecordOptionalMetricDataPoint(ts pcommon.Timestamp, val float64, stringAttrAttributeValue string, booleanAttrAttributeValue bool, booleanAttr2AttributeValue bool) { + mb.metricOptionalMetric.recordDataPoint(mb.startTime, ts, val, stringAttrAttributeValue, booleanAttrAttributeValue, booleanAttr2AttributeValue) } // RecordOptionalMetricEmptyUnitDataPoint adds a data point to optional.metric.empty_unit metric. diff --git a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go index f4100521330..61d7f4c9caf 100644 --- a/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go +++ b/cmd/mdatagen/internal/samplereceiver/internal/metadata/generated_metrics_test.go @@ -110,7 +110,7 @@ func TestMetricsBuilder(t *testing.T) { mb.RecordMetricInputTypeDataPoint(ts, "1", "string_attr-val", 19, AttributeEnumAttrRed, []any{"slice_attr-item1", "slice_attr-item2"}, map[string]any{"key1": "map_attr-val1", "key2": "map_attr-val2"}) allMetricsCount++ - mb.RecordOptionalMetricDataPoint(ts, 1, "string_attr-val", true) + mb.RecordOptionalMetricDataPoint(ts, 1, "string_attr-val", true, false) allMetricsCount++ mb.RecordOptionalMetricEmptyUnitDataPoint(ts, 1, "string_attr-val", true) @@ -236,6 +236,9 @@ func TestMetricsBuilder(t *testing.T) { attrVal, ok = dp.Attributes().Get("boolean_attr") assert.True(t, ok) assert.True(t, attrVal.Bool()) + attrVal, ok = dp.Attributes().Get("boolean_attr2") + assert.True(t, ok) + assert.False(t, attrVal.Bool()) case "optional.metric.empty_unit": assert.False(t, validatedMetrics["optional.metric.empty_unit"], "Found a duplicate in the metrics slice: optional.metric.empty_unit") validatedMetrics["optional.metric.empty_unit"] = true diff --git a/cmd/mdatagen/internal/samplereceiver/metadata.yaml b/cmd/mdatagen/internal/samplereceiver/metadata.yaml index 5a6df8fb734..51548d18511 100644 --- a/cmd/mdatagen/internal/samplereceiver/metadata.yaml +++ b/cmd/mdatagen/internal/samplereceiver/metadata.yaml @@ -43,27 +43,27 @@ resource_attributes: map.resource.attr: description: Resource attribute with a map value. - type: map + type: map enabled: true string.resource.attr_disable_warning: description: Resource attribute with any string value. type: string - enabled: true + enabled: true warnings: if_enabled_not_set: This resource_attribute will be disabled by default soon. string.resource.attr_remove_warning: description: Resource attribute with any string value. type: string - enabled: false + enabled: false warnings: if_configured: This resource_attribute is deprecated and will be removed soon. string.resource.attr_to_be_removed: description: Resource attribute with any string value. type: string - enabled: true + enabled: true warnings: if_enabled: This resource_attribute is deprecated and will be removed soon. @@ -86,6 +86,12 @@ attributes: description: Attribute with a boolean value. type: bool + # This 2nd boolean attribute allows us to test both values for boolean attributes, + # as test values are based on the parity of the attribute name length. + boolean_attr2: + description: Another attribute with a boolean value. + type: bool + slice_attr: description: Attribute with a slice value. type: slice @@ -114,7 +120,7 @@ metrics: unit: "1" gauge: value_type: double - attributes: [string_attr, boolean_attr] + attributes: [string_attr, boolean_attr, boolean_attr2] warnings: if_configured: This metric is deprecated and will be removed soon. diff --git a/cmd/mdatagen/loader_test.go b/cmd/mdatagen/loader_test.go index 7d6927d3105..68dcfb84e0f 100644 --- a/cmd/mdatagen/loader_test.go +++ b/cmd/mdatagen/loader_test.go @@ -149,6 +149,13 @@ func TestLoadMetadata(t *testing.T) { }, FullName: "boolean_attr", }, + "boolean_attr2": { + Description: "Another attribute with a boolean value.", + Type: ValueType{ + ValueType: pcommon.ValueTypeBool, + }, + FullName: "boolean_attr2", + }, "slice_attr": { Description: "Attribute with a slice value.", Type: ValueType{ @@ -190,7 +197,7 @@ func TestLoadMetadata(t *testing.T) { Gauge: &gauge{ MetricValueType: MetricValueType{pmetric.NumberDataPointValueTypeDouble}, }, - Attributes: []attributeName{"string_attr", "boolean_attr"}, + Attributes: []attributeName{"string_attr", "boolean_attr", "boolean_attr2"}, }, "optional.metric.empty_unit": { Enabled: false, diff --git a/cmd/mdatagen/templates/metrics_test.go.tmpl b/cmd/mdatagen/templates/metrics_test.go.tmpl index ecb7008960e..ce9931227e9 100644 --- a/cmd/mdatagen/templates/metrics_test.go.tmpl +++ b/cmd/mdatagen/templates/metrics_test.go.tmpl @@ -179,7 +179,7 @@ func TestMetricsBuilder(t *testing.T) { attrVal, ok {{ if eq $i 0 }}:{{ end }}= dp.Attributes().Get("{{ (attributeInfo $attr).Name }}") assert.True(t, ok) {{- if eq (attributeInfo $attr).Type.String "Bool"}} - assert.{{- if (attributeInfo $attr).TestValue }}True{{ else }}False{{- end }}(t, attrVal.{{ (attributeInfo $attr).Type }}() + assert.{{- if eq (attributeInfo $attr).TestValue "true" }}True{{ else }}False{{- end }}(t, attrVal.{{ (attributeInfo $attr).Type }}() {{- else }} assert.EqualValues(t, {{ (attributeInfo $attr).TestValue }}, attrVal.{{ (attributeInfo $attr).Type }}() {{- end }}