fix(go): drop invalid enum values instead of recording empty attributes

buildscripts/check_go_output.sh

@@ -40,6 +40,16 @@ for test_dir in "${PACKAGE_DIR}"/tests/*/; do
   mkdir -p "${package_dir}"
   cp -R "${expected}/." "${package_dir}/"
 
+  # Hand-written consumer tests exercise the generated API the way a user of
+  # it would. They mirror expected's package layout, e.g.
+  # consumer/myappattr/attributes_test.go sits beside expected/myappattr, so
+  # they land in the same package directory and are gofmt-checked and
+  # type-checked alongside the generated code.
+  consumer="${test_dir}consumer"
+  if [[ -d "${consumer}" ]]; then
+    cp -R "${consumer}/." "${package_dir}/"
+  fi
+
   # A test that maps an attribute namespace to a dependency registry, via
   # `imported_import_paths`, needs that dependency actually generated into the
   # module too, so the typed references it emits resolve to a real package.
@@ -74,9 +84,65 @@ for test_dir in "${PACKAGE_DIR}"/tests/*/; do
     cd "${module_dir}"
     go mod init "${module_path}" >/dev/null 2>&1
     go get "go.opentelemetry.io/otel@${OTEL_GO_VERSION}" "go.opentelemetry.io/otel/metric@${OTEL_GO_VERSION}" "go.opentelemetry.io/otel/trace@${OTEL_GO_VERSION}" "go.opentelemetry.io/otel/log@${OTEL_GO_LOG_VERSION}" "go.opentelemetry.io/otel/sdk@${OTEL_GO_VERSION}" 2>&1 | grep -v "^go: " || true
+    if [[ -d "${consumer}" ]]; then
+      go get "go.opentelemetry.io/otel/sdk/metric@${OTEL_GO_VERSION}" "go.opentelemetry.io/otel/sdk/log@${OTEL_GO_LOG_VERSION}" 2>&1 | grep -v "^go: " || true
+    fi
     go mod tidy
     go build ./...
     go vet ./...
   ) || { echo "  FAIL: ${test_name} does not build."; exit 1; }
   echo "  PASS: ${test_name} builds and vets."
+
+  if [[ -d "${consumer}" ]]; then
+    (
+      cd "${module_dir}"
+      go test ./...
+    ) || { echo "  FAIL: ${test_name} consumer tests failed."; exit 1; }
+    echo "  PASS: ${test_name} consumer tests pass."
+  fi
+
+  compile_fail="${test_dir}compile-fail"
+  if [[ -d "${compile_fail}" ]]; then
+    for invalid_file in "${compile_fail}"/*.go; do
+      [[ -e "${invalid_file}" ]] || continue
+      case_name="$(basename "${invalid_file}" .go)"
+      expected_error="${compile_fail}/${case_name}.expected-error.txt"
+      if [[ ! -s "${expected_error}" ]]; then
+        echo "  FAIL: ${test_name}/compile-fail/${case_name}.go has no ${case_name}.expected-error.txt, or it is empty."
+        exit 1
+      fi
+
+      cf_module_dir="${WORK_DIR}/${test_name}-compile-fail-${case_name}"
+      cf_package_dir="${cf_module_dir}${import_path#"${module_path}"}"
+      mkdir -p "${cf_package_dir}"
+      cp -R "${expected}/." "${cf_package_dir}/"
+      if [[ -f "${dependency_params}" ]]; then
+        cf_dependency_dir="${cf_module_dir}${dependency_import_path#"${module_path}"}"
+        mkdir -p "${cf_dependency_dir}"
+        cp -R "${dependency_observed}/." "${cf_dependency_dir}/"
+      fi
+      invalid_dir="${cf_module_dir}/invalidconsumer"
+      mkdir -p "${invalid_dir}"
+      cp "${invalid_file}" "${invalid_dir}/$(basename "${invalid_file}")"
+
+      diagnostics="${WORK_DIR}/${test_name}-${case_name}.diagnostics.txt"
+      (
+        cd "${cf_module_dir}"
+        go mod init "${module_path}" >/dev/null 2>&1
+        go get "go.opentelemetry.io/otel@${OTEL_GO_VERSION}" "go.opentelemetry.io/otel/metric@${OTEL_GO_VERSION}" "go.opentelemetry.io/otel/trace@${OTEL_GO_VERSION}" "go.opentelemetry.io/otel/log@${OTEL_GO_LOG_VERSION}" "go.opentelemetry.io/otel/sdk@${OTEL_GO_VERSION}" 2>&1 | grep -v "^go: " || true
+        go mod tidy >/dev/null 2>&1 || true
+      ) || { echo "  FAIL: ${test_name}/compile-fail/${case_name}.go: could not set up its module."; exit 1; }
+
+      if ( cd "${cf_module_dir}" && go build ./... >"${diagnostics}" 2>&1 ); then
+        echo "  FAIL: ${test_name}/compile-fail/${case_name}.go built, but it must not."
+        exit 1
+      fi
+      if ! grep -Fqf "${expected_error}" "${diagnostics}"; then
+        echo "  FAIL: ${test_name}/compile-fail/${case_name}.go failed, but not with the expected error:"
+        cat "${diagnostics}"
+        exit 1
+      fi
+      echo "  PASS: ${test_name}/compile-fail/${case_name}.go fails with the expected error."
+    done
+  fi
 done

templates/code/go/README.md

@@ -73,6 +73,15 @@ enums of those. An enum takes the Go type of its members: `string`, `int64`,
 `float64` when any member is fractional, or `bool`. Template and `any` types are
 not rendered.
 
+An enum's struct carries a hidden field marking it as one of the package's
+members, so the zero value of an enum type, such as `myappattr.TaskState{}`, is
+never a valid member even when an enum's value type makes the zero value look
+otherwise valid, as a `boolean` enum with a `false` member does. `KeyValue`, and
+every metric, span, event, and entity that records the attribute, detects the
+zero value and drops the attribute instead of emitting one with an empty value.
+The OpenTelemetry Go SDK drops invalid attributes the same way, and
+`otel.Handle` is reserved for irremediable errors, so nothing is reported.
+
 ### Root package
 
 The root package is named after the last element of `import_path`, as Go

templates/code/go/attributes.go.j2

@@ -17,7 +17,14 @@ import "go.opentelemetry.io/otel/attribute"
 const {{ key }} = attribute.Key("{{ attr.key }}")
 
 {{ [name ~ " is a value of `" ~ attr.key ~ "`.", attr.brief, attr.note, deprecated] | select | join("\n\n") | comment }}
+{%- if is_enum %}
+type {{ name }} struct {
+	value {{ attr.value_type | map_text("attribute_field_type") }}
+	ok    bool
+}
+{%- else %}
 type {{ name }} struct{ value {{ attr.value_type | map_text("attribute_field_type") }} }
+{%- endif %}
 {% if not is_enum %}
 {{ ["New" ~ name ~ " returns a value of `" ~ attr.key ~ "`.", deprecated] | select | join("\n\n") | comment }}
 func New{{ name }}(value {{ attr.value_type | map_text("attribute_type_value") }}) {{ name }} {
@@ -28,19 +35,28 @@ func New{{ name }}(value {{ attr.value_type | map_text("attribute_type_value") }
 func ({{ name }}) Key() attribute.Key {
 	return {{ key }}
 }
-
+{% if is_enum %}
+{{ ["KeyValue is the `" ~ attr.key ~ "` attribute set to this value.", "The zero " ~ name ~ " is not a valid value. KeyValue returns an invalid attribute.KeyValue for it, which the generated instruments, spans, events, and entities drop."] | select | join("\n\n") | comment }}
+func (v {{ name }}) KeyValue() attribute.KeyValue {
+	if !v.ok {
+		return attribute.KeyValue{}
+	}
+	return {{ key }}.{{ attr.value_type | map_text("attribute_type_method") }}(v.value)
+}
+{%- else %}
 {{ ["KeyValue is the `" ~ attr.key ~ "` attribute set to this value."] | join | comment }}
 func (v {{ name }}) KeyValue() attribute.KeyValue {
 	return {{ key }}.{{ attr.value_type | map_text("attribute_type_method") }}(v.value)
 }
+{%- endif %}
 {% if is_enum %}
 var (
 {% for member in attr.type.members %}
 {%- if not loop.first %}
 
 {% endif -%}
 {{ [base ~ n.exported(member.id) ~ " is the " ~ (member.value | print_member_value) ~ " member of `" ~ attr.key ~ "`.", member.brief, member.note, d.notice(member) or deprecated] | select | join("\n\n") | comment(format="go_indented") }}
-	{{ base }}{{ n.exported(member.id) }} = {{ name }}{value: {{ member.value | print_member_value }}}
+	{{ base }}{{ n.exported(member.id) }} = {{ name }}{value: {{ member.value | print_member_value }}, ok: true}
 {%- endfor %}
 )
 {% endif -%}

templates/code/go/entities.go.j2

@@ -64,11 +64,19 @@ func New{{ entity }}({% for req in def.required %}{% if not loop.first %}, {% en
 {%- if def.required or def.optional %}
 	attrs := make([]attribute.KeyValue, 0, {{ def.required | length }}{% if def.optional %}+len(opts){% endif %})
 {%- for req in def.required %}
+{%- if req.imported %}
 	attrs = append(attrs, {{ attr_kv(req, n.unexported(req.attr_param)) }})
+{%- else %}
+	if kv := {{ n.unexported(req.attr_param) }}.KeyValue(); kv.Valid() {
+		attrs = append(attrs, kv)
+	}
+{%- endif %}
 {%- endfor %}
 {%- if def.optional %}
 	for _, opt := range opts {
-		attrs = append(attrs, opt.{{ private }}EntityAttr())
+		if kv := opt.{{ private }}EntityAttr(); kv.Valid() {
+			attrs = append(attrs, kv)
+		}
 	}
 {%- endif %}
 	return {{ entity }}{attrs: attrs}

templates/code/go/events.go.j2

@@ -70,7 +70,13 @@ func With{{ name }}Timestamp(timestamp time.Time) {{ option }} {
 type {{ private }}Event{{ optname }} struct{ v {{ attr_type(opt) }} }
 
 func (o {{ private }}Event{{ optname }}) {{ private }}Option(record *log.Record) {
+{%- if opt.imported %}
 	record.AddAttributes({{ attr_kv(opt, "o.v") }})
+{%- else %}
+	if kv := o.v.KeyValue(); kv.Valid() {
+		record.AddAttributes(kv)
+	}
+{%- endif %}
 }
 
 {{ ["With" ~ name ~ optname ~ " sets `" ~ opt.key ~ "` on the event.", opt.requirement_doc] | select | join("\n\n") | comment }}
@@ -84,11 +90,17 @@ func {{ emit }}(ctx context.Context, logger {{ n.root() }}logger.Logger{% for re
 	record.SetEventName({{ def.name | tojson }})
 	record.SetSeverity(log.SeverityInfo)
 {%- if def.required %}
-	record.AddAttributes(
+	kvs := make([]attribute.KeyValue, 0, {{ def.required | length }})
 {%- for req in def.required %}
-		{{ attr_kv(req, n.unexported(req.attr_param)) }},
+{%- if req.imported %}
+	kvs = append(kvs, {{ attr_kv(req, n.unexported(req.attr_param)) }})
+{%- else %}
+	if kv := {{ n.unexported(req.attr_param) }}.KeyValue(); kv.Valid() {
+		kvs = append(kvs, kv)
+	}
+{%- endif %}
 {%- endfor %}
-	)
+	record.AddAttributes(kvs...)
 {%- endif %}
 	for _, opt := range opts {
 		opt.{{ private }}Option(&record)

templates/code/go/metrics.go.j2

@@ -109,11 +109,19 @@ func (m {{ name }}) {{ method }}(ctx context.Context, {{ arg }} {{ gotype }}{% f
 {%- if def.required or def.optional %}
 	kvs := make([]attribute.KeyValue, 0, {{ def.required | length }}{% if def.optional %}+len(opts){% endif %})
 {%- for req in def.required %}
+{%- if req.imported %}
 	kvs = append(kvs, {{ attr_kv(req, n.unexported(req.attr_param)) }})
+{%- else %}
+	if kv := {{ n.unexported(req.attr_param) }}.KeyValue(); kv.Valid() {
+		kvs = append(kvs, kv)
+	}
+{%- endif %}
 {%- endfor %}
 {%- if def.optional %}
 	for _, opt := range opts {
-		kvs = append(kvs, opt.{{ private }}Attr())
+		if kv := opt.{{ private }}Attr(); kv.Valid() {
+			kvs = append(kvs, kv)
+		}
 	}
 {%- endif %}
 	m.inst.{{ method }}(ctx, {{ arg }}, metric.WithAttributes(kvs...))
@@ -139,11 +147,19 @@ func (o {{ observer }}) Observe(value {{ gotype }}{% for req in def.required %},
 {%- if def.required or def.optional %}
 	kvs := make([]attribute.KeyValue, 0, {{ def.required | length }}{% if def.optional %}+len(opts){% endif %})
 {%- for req in def.required %}
+{%- if req.imported %}
 	kvs = append(kvs, {{ attr_kv(req, n.unexported(req.attr_param)) }})
+{%- else %}
+	if kv := {{ n.unexported(req.attr_param) }}.KeyValue(); kv.Valid() {
+		kvs = append(kvs, kv)
+	}
+{%- endif %}
 {%- endfor %}
 {%- if def.optional %}
 	for _, opt := range opts {
-		kvs = append(kvs, opt.{{ private }}Attr())
+		if kv := opt.{{ private }}Attr(); kv.Valid() {
+			kvs = append(kvs, kv)
+		}
 	}
 {%- endif %}
 	o.o.Observe(value, metric.WithAttributes(kvs...))

templates/code/go/spans.go.j2

@@ -102,7 +102,9 @@ func (s {{ span }}) End(opts ...trace.SpanEndOption) {
 func (s {{ span }}) SetAttributes(attrs ...{{ name }}Attr) {
 	kvs := make([]attribute.KeyValue, 0, len(attrs))
 	for _, attr := range attrs {
-		kvs = append(kvs, attr.{{ private }}StartAttr())
+		if kv := attr.{{ private }}StartAttr(); kv.Valid() {
+			kvs = append(kvs, kv)
+		}
 	}
 	s.Span().SetAttributes(kvs...)
 }
@@ -128,11 +130,19 @@ func {{ start }}(ctx context.Context, tracer {{ n.root() }}tracer.Tracer{% if no
 {%- if def.required or def.optional %}
 	kvs := make([]attribute.KeyValue, 0, {{ def.required | length }}{% if def.optional %}+len(opts){% endif %})
 {%- for req in def.required %}
+{%- if req.imported %}
 	kvs = append(kvs, {{ attr_kv(req, n.unexported(req.attr_param)) }})
+{%- else %}
+	if kv := {{ n.unexported(req.attr_param) }}.KeyValue(); kv.Valid() {
+		kvs = append(kvs, kv)
+	}
+{%- endif %}
 {%- endfor %}
 {%- if def.optional %}
 	for _, opt := range opts {
-		kvs = append(kvs, opt.{{ private }}StartAttr())
+		if kv := opt.{{ private }}StartAttr(); kv.Valid() {
+			kvs = append(kvs, kv)
+		}
 	}
 {%- endif %}
 {%- endif %}

templates/code/go/tests/attributes/compile-fail/enum_from_outside_package.expected-error.txt

@@ -0,0 +1 @@
+cannot refer to unexported field value in struct literal of type myappattr.TaskLane

templates/code/go/tests/attributes/compile-fail/enum_from_outside_package.go

@@ -0,0 +1,5 @@
+package invalidconsumer
+
+import "example.com/semconv/myappattr"
+
+var invalid = myappattr.TaskLane{value: "fast"}

templates/code/go/tests/attributes/consumer/myappattr/attributes_test.go

@@ -0,0 +1,59 @@
+package myappattr_test
+
+import (
+	"testing"
+
+	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/attribute"
+
+	"example.com/semconv/myappattr"
+)
+
+func TestKeyValue(t *testing.T) {
+	kv := myappattr.NewTaskID("t-1").KeyValue()
+	want := attribute.String("myapp.task.id", "t-1")
+	if kv != want {
+		t.Fatalf("KeyValue() = %v, want %v", kv, want)
+	}
+}
+
+func TestEnumMemberKeyValue(t *testing.T) {
+	kv := myappattr.TaskStateRunning.KeyValue()
+	want := attribute.String("myapp.task.state", "running")
+	if kv != want {
+		t.Fatalf("KeyValue() = %v, want %v", kv, want)
+	}
+}
+
+func TestZeroValueEnumDropsWithoutReporting(t *testing.T) {
+	var reported error
+	otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) { reported = err }))
+	t.Cleanup(func() { otel.SetErrorHandler(otel.ErrorHandlerFunc(func(error) {})) })
+
+	kv := myappattr.TaskState{}.KeyValue()
+
+	if kv.Valid() {
+		t.Fatalf("KeyValue() = %v, want an invalid attribute.KeyValue", kv)
+	}
+	if reported != nil {
+		t.Fatalf("the zero TaskState reported %v, want nothing reported", reported)
+	}
+}
+
+func TestZeroValueBooleanEnumIsNotItsFalseMember(t *testing.T) {
+	var reported error
+	otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) { reported = err }))
+	t.Cleanup(func() { otel.SetErrorHandler(otel.ErrorHandlerFunc(func(error) {})) })
+
+	if kv := myappattr.TaskPreemptibleForbidden.KeyValue(); !kv.Valid() || kv.Value.AsBool() {
+		t.Fatalf("TaskPreemptibleForbidden.KeyValue() = %v, want a valid false attribute", kv)
+	}
+
+	zero := myappattr.TaskPreemptible{}.KeyValue()
+	if zero.Valid() {
+		t.Fatalf("the zero TaskPreemptible.KeyValue() = %v, want an invalid attribute", zero)
+	}
+	if reported != nil {
+		t.Fatalf("the zero TaskPreemptible reported %v, want nothing reported", reported)
+	}
+}