From c48eddc7364e72fbf0ec8218000f7177f08fccb5 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Thu, 18 Aug 2022 10:19:17 -0700 Subject: [PATCH] Remove unnecessary duplicate code and allocations for reading enums in JSON (#5928) Signed-off-by: Bogdan Signed-off-by: Bogdan --- CHANGELOG.md | 1 + pdata/internal/json/enum.go | 40 +++++++++++++++ pdata/internal/json/enum_test.go | 85 ++++++++++++++++++++++++++++++++ pdata/pmetric/json.go | 7 +-- pdata/pmetric/json_test.go | 44 ----------------- pdata/ptrace/json.go | 22 +-------- pdata/ptrace/json_test.go | 58 ---------------------- 7 files changed, 129 insertions(+), 128 deletions(-) create mode 100644 pdata/internal/json/enum.go create mode 100644 pdata/internal/json/enum_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 51f98cd48f0..65078b86593 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ - Add support to unmarshalls bytes into pmetric.Metrics with `jsoniter` in jsonUnmarshaler(#5433) - Add httpprovider to allow loading config files stored in HTTP (#5810) - Added `service.telemetry.traces.propagators` configuration to set propagators for collector's internal spans. (#5572) +- Remove unnecessary duplicate code and allocations for reading enums in JSON. (#5928) ### 🧰 Bug fixes 🧰 diff --git a/pdata/internal/json/enum.go b/pdata/internal/json/enum.go new file mode 100644 index 00000000000..160bc5fb51e --- /dev/null +++ b/pdata/internal/json/enum.go @@ -0,0 +1,40 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed 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 json // import "go.opentelemetry.io/collector/pdata/internal/json" + +import ( + jsoniter "github.com/json-iterator/go" +) + +// ReadEnumValue returns the enum integer value representation. Accepts both enum names and enum integer values. +// See https://developers.google.com/protocol-buffers/docs/proto3#json. +func ReadEnumValue(iter *jsoniter.Iterator, valueMap map[string]int32) int32 { + switch iter.WhatIsNext() { + case jsoniter.NumberValue: + return iter.ReadInt32() + case jsoniter.StringValue: + val, ok := valueMap[iter.ReadString()] + // Same behavior with official protbuf JSON decoder, + // see https://github.com/open-telemetry/opentelemetry-proto-go/pull/81 + if !ok { + iter.ReportError("ReadEnumValue", "unknown string value") + return 0 + } + return val + default: + iter.ReportError("ReadEnumValue", "unsupported value type") + return 0 + } +} diff --git a/pdata/internal/json/enum_test.go b/pdata/internal/json/enum_test.go new file mode 100644 index 00000000000..0d939f62af7 --- /dev/null +++ b/pdata/internal/json/enum_test.go @@ -0,0 +1,85 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed 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 json + +import ( + "testing" + + jsoniter "github.com/json-iterator/go" + "github.com/stretchr/testify/assert" +) + +func TestReadEnumValue(t *testing.T) { + valueMap := map[string]int32{ + "undefined": 0, + "foo": 1, + "bar": 2, + } + tests := []struct { + name string + jsonStr string + want int32 + wantErr bool + }{ + { + name: "foo string", + jsonStr: "\"foo\"\n", + want: 1, + }, + { + name: "foo number", + jsonStr: "1\n", + want: 1, + }, + { + name: "unknown number", + jsonStr: "5\n", + want: 5, + }, + { + name: "bar string", + jsonStr: "\"bar\"\n", + want: 2, + }, + { + name: "bar number", + jsonStr: "2\n", + want: 2, + }, + { + name: "unknown string", + jsonStr: "\"baz\"\n", + wantErr: true, + }, + { + name: "wrong type", + jsonStr: "true", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + iter := jsoniter.ConfigFastest.BorrowIterator([]byte(tt.jsonStr)) + defer jsoniter.ConfigFastest.ReturnIterator(iter) + val := ReadEnumValue(iter, valueMap) + if tt.wantErr { + assert.Error(t, iter.Error) + return + } + assert.NoError(t, iter.Error) + assert.Equal(t, tt.want, val) + }) + } +} diff --git a/pdata/pmetric/json.go b/pdata/pmetric/json.go index 9ef72f6b902..f8aeb6919f0 100644 --- a/pdata/pmetric/json.go +++ b/pdata/pmetric/json.go @@ -527,10 +527,5 @@ func (d *jsonUnmarshaler) readQuantileValue(iter *jsoniter.Iterator) *otlpmetric } func (d *jsonUnmarshaler) readAggregationTemporality(iter *jsoniter.Iterator) otlpmetrics.AggregationTemporality { - value := iter.ReadAny() - if v := value.ToInt(); v > 0 { - return otlpmetrics.AggregationTemporality(v) - } - v := value.ToString() - return otlpmetrics.AggregationTemporality(otlpmetrics.AggregationTemporality_value[v]) + return otlpmetrics.AggregationTemporality(json.ReadEnumValue(iter, otlpmetrics.AggregationTemporality_value)) } diff --git a/pdata/pmetric/json_test.go b/pdata/pmetric/json_test.go index 16ba801eb17..74f4106cd8e 100644 --- a/pdata/pmetric/json_test.go +++ b/pdata/pmetric/json_test.go @@ -15,7 +15,6 @@ package pmetric import ( - "fmt" "testing" "time" @@ -334,49 +333,6 @@ func Test_jsonUnmarshaler_UnmarshalMetrics(t *testing.T) { } } -func TestReadAggregationTemporality(t *testing.T) { - tests := []struct { - name string - jsonStr string - want otlpmetrics.AggregationTemporality - }{ - { - name: "string", - jsonStr: fmt.Sprintf(`"%s"`, - otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_CUMULATIVE.String()), - want: otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_CUMULATIVE, - }, - { - name: "string", - jsonStr: fmt.Sprintf(`"%s"`, - otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_DELTA.String()), - want: otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_DELTA, - }, - { - name: "int", - jsonStr: fmt.Sprintf("%d", - otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_CUMULATIVE), - want: otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_CUMULATIVE, - }, - { - name: "int", - jsonStr: fmt.Sprintf("%d", - otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_DELTA), - want: otlpmetrics.AggregationTemporality_AGGREGATION_TEMPORALITY_DELTA, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - iter := jsoniter.ConfigFastest.BorrowIterator([]byte(tt.jsonStr)) - defer jsoniter.ConfigFastest.ReturnIterator(iter) - unmarshaler := &jsonUnmarshaler{} - if got := unmarshaler.readAggregationTemporality(iter); got != tt.want { - t.Errorf("readSpanKind() = %v, want %v", got, tt.want) - } - }) - } -} - func TestReadMetricsDataUnknownField(t *testing.T) { jsonStr := `{"extra":""}` iter := jsoniter.ConfigFastest.BorrowIterator([]byte(jsonStr)) diff --git a/pdata/ptrace/json.go b/pdata/ptrace/json.go index 7f2c2595919..f88c0a507ac 100644 --- a/pdata/ptrace/json.go +++ b/pdata/ptrace/json.go @@ -158,7 +158,7 @@ func readSpan(iter *jsoniter.Iterator) *otlptrace.Span { case "name": sp.Name = iter.ReadString() case "kind": - sp.Kind = readSpanKind(iter) + sp.Kind = otlptrace.Span_SpanKind(json.ReadEnumValue(iter, otlptrace.Span_SpanKind_value)) case "startTimeUnixNano", "start_time_unix_nano": sp.StartTimeUnixNano = json.ReadUint64(iter) case "endTimeUnixNano", "end_time_unix_nano": @@ -190,7 +190,7 @@ func readSpan(iter *jsoniter.Iterator) *otlptrace.Span { case "message": sp.Status.Message = iter.ReadString() case "code": - sp.Status.Code = readStatusCode(iter) + sp.Status.Code = otlptrace.Status_StatusCode(json.ReadEnumValue(iter, otlptrace.Status_StatusCode_value)) default: iter.ReportError("readSpan.status", fmt.Sprintf("unknown field:%v", f)) } @@ -257,21 +257,3 @@ func readSpanEvent(iter *jsoniter.Iterator) *otlptrace.Span_Event { }) return event } - -func readSpanKind(iter *jsoniter.Iterator) otlptrace.Span_SpanKind { - any := iter.ReadAny() - if v := any.ToInt(); v > 0 { - return otlptrace.Span_SpanKind(v) - } - v := any.ToString() - return otlptrace.Span_SpanKind(otlptrace.Span_SpanKind_value[v]) -} - -func readStatusCode(iter *jsoniter.Iterator) otlptrace.Status_StatusCode { - any := iter.ReadAny() - if v := any.ToInt(); v > 0 { - return otlptrace.Status_StatusCode(v) - } - v := any.ToString() - return otlptrace.Status_StatusCode(otlptrace.Status_StatusCode_value[v]) -} diff --git a/pdata/ptrace/json_test.go b/pdata/ptrace/json_test.go index 46e5089ff52..7c3c4f0323d 100644 --- a/pdata/ptrace/json_test.go +++ b/pdata/ptrace/json_test.go @@ -15,7 +15,6 @@ package ptrace import ( - "fmt" "testing" "time" @@ -23,7 +22,6 @@ import ( "github.com/stretchr/testify/assert" "go.opentelemetry.io/collector/pdata/internal" - otlptrace "go.opentelemetry.io/collector/pdata/internal/data/protogen/trace/v1" ) var tracesOTLP = func() Traces { @@ -285,59 +283,3 @@ func TestReadSpanEventUnknownField(t *testing.T) { assert.Contains(t, iter.Error.Error(), "unknown field") } } - -func TestReadSpanKind(t *testing.T) { - tests := []struct { - name string - jsonStr string - want otlptrace.Span_SpanKind - }{ - { - name: "string", - jsonStr: fmt.Sprintf(`"%s"`, otlptrace.Span_SPAN_KIND_INTERNAL.String()), - want: otlptrace.Span_SPAN_KIND_INTERNAL, - }, - { - name: "int", - jsonStr: fmt.Sprintf("%d", otlptrace.Span_SPAN_KIND_INTERNAL), - want: otlptrace.Span_SPAN_KIND_INTERNAL, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - iter := jsoniter.ConfigFastest.BorrowIterator([]byte(tt.jsonStr)) - defer jsoniter.ConfigFastest.ReturnIterator(iter) - if got := readSpanKind(iter); got != tt.want { - t.Errorf("readSpanKind() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestReadStatusCode(t *testing.T) { - tests := []struct { - name string - jsonStr string - want otlptrace.Status_StatusCode - }{ - { - name: "string", - jsonStr: fmt.Sprintf(`"%s"`, otlptrace.Status_STATUS_CODE_ERROR.String()), - want: otlptrace.Status_STATUS_CODE_ERROR, - }, - { - name: "int", - jsonStr: fmt.Sprintf("%d", otlptrace.Status_STATUS_CODE_ERROR), - want: otlptrace.Status_STATUS_CODE_ERROR, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - iter := jsoniter.ConfigFastest.BorrowIterator([]byte(tt.jsonStr)) - defer jsoniter.ConfigFastest.ReturnIterator(iter) - if got := readStatusCode(iter); got != tt.want { - t.Errorf("readStatusCode() = %v, want %v", got, tt.want) - } - }) - } -}