From 07db55318268bebf30eaa4dea986d96cafc7b1a3 Mon Sep 17 00:00:00 2001 From: Tigran Najaryan Date: Tue, 1 Sep 2020 13:13:08 -0400 Subject: [PATCH] Make pdata.LogsToOtlp and pdata.LogsFromOtlp impossible to call from outside this module LogsToOtlp and LogsFromOtlp are intended to be used only by OTLP exporter (for which OTLP is the native data format) and fileexporter (for which we do want to output internal representation for debugging purposes). All other code is supposed to use public member functions of pdata.Logs struct. This changes makes it impossible to use LogsToOtlp and LogsFromOtlp outside this module by hiding them behind an intermediary struct that is declared in an internal package. This is necessary to prevent accidental use of LogsToOtlp and LogsFromOtlp and the OTLP data structs, which we prohibit to do since the OTLP data structs may change in the future. --- consumer/pdata/log.go | 27 ++++++++++------- consumer/pdata/log_test.go | 15 +++++----- exporter/fileexporter/file_exporter.go | 3 +- exporter/fileexporter/file_exporter_test.go | 5 ++-- exporter/otlpexporter/otlp.go | 3 +- internal/data/testdata/log.go | 7 +++-- internal/data/testdata/log_test.go | 5 ++-- internal/empty_test.go | 15 ++++++++++ internal/otlp_wrapper.go | 32 +++++++++++++++++++++ receiver/otlpreceiver/logs/otlp.go | 3 +- receiver/otlpreceiver/logs/otlp_test.go | 3 +- 11 files changed, 89 insertions(+), 29 deletions(-) create mode 100644 internal/empty_test.go create mode 100644 internal/otlp_wrapper.go diff --git a/consumer/pdata/log.go b/consumer/pdata/log.go index e97c1ea17d1..88682f06971 100644 --- a/consumer/pdata/log.go +++ b/consumer/pdata/log.go @@ -17,6 +17,7 @@ package pdata import ( "github.com/gogo/protobuf/proto" + "go.opentelemetry.io/collector/internal" otlplogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/logs/v1" ) @@ -32,31 +33,35 @@ type Logs struct { orig *[]*otlplogs.ResourceLogs } -// LogsFromOtlp creates the internal Logs representation from the ProtoBuf. -func LogsFromOtlp(orig []*otlplogs.ResourceLogs) Logs { +// NewLogs creates a new Logs. +func NewLogs() Logs { + orig := []*otlplogs.ResourceLogs(nil) return Logs{&orig} } -// LogsToOtlp converts the internal Logs to the ProtoBuf. -func LogsToOtlp(ld Logs) []*otlplogs.ResourceLogs { - return *ld.orig +// LogsFromInternalRep creates the internal Logs representation from the ProtoBuf. Should +// not be used outside this module. This is intended to be used only by OTLP exporter and +// File exporter, which legitimately need to work with OTLP Protobuf structs. +func LogsFromInternalRep(logs internal.OtlpLogsWrapper) Logs { + return Logs{logs.Orig} } -// NewLogs creates a new Logs. -func NewLogs() Logs { - orig := []*otlplogs.ResourceLogs(nil) - return Logs{&orig} +// InternalRep returns internal representation of the logs. Should not be used outside +// this module. This is intended to be used only by OTLP exporter and File exporter, +// which legitimately need to work with OTLP Protobuf structs. +func (ld Logs) InternalRep() internal.OtlpLogsWrapper { + return internal.OtlpLogsWrapper{Orig: ld.orig} } // Clone returns a copy of Logs. func (ld Logs) Clone() Logs { - otlp := LogsToOtlp(ld) + otlp := *ld.orig resourceSpansClones := make([]*otlplogs.ResourceLogs, 0, len(otlp)) for _, resourceSpans := range otlp { resourceSpansClones = append(resourceSpansClones, proto.Clone(resourceSpans).(*otlplogs.ResourceLogs)) } - return LogsFromOtlp(resourceSpansClones) + return Logs{orig: &resourceSpansClones} } // LogRecordCount calculates the total number of log records. diff --git a/consumer/pdata/log_test.go b/consumer/pdata/log_test.go index b1636edff3d..e4eb098fbef 100644 --- a/consumer/pdata/log_test.go +++ b/consumer/pdata/log_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/assert" + "go.opentelemetry.io/collector/internal" otlplogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/logs/v1" ) @@ -46,13 +47,13 @@ func TestLogRecordCount(t *testing.T) { } func TestLogRecordCountWithNils(t *testing.T) { - assert.EqualValues(t, 0, LogsFromOtlp([]*otlplogs.ResourceLogs{nil, {}}).LogRecordCount()) - assert.EqualValues(t, 0, LogsFromOtlp([]*otlplogs.ResourceLogs{ + assert.EqualValues(t, 0, LogsFromInternalRep(internal.LogsFromOtlp([]*otlplogs.ResourceLogs{nil, {}})).LogRecordCount()) + assert.EqualValues(t, 0, LogsFromInternalRep(internal.LogsFromOtlp([]*otlplogs.ResourceLogs{ { InstrumentationLibraryLogs: []*otlplogs.InstrumentationLibraryLogs{nil, {}}, }, - }).LogRecordCount()) - assert.EqualValues(t, 2, LogsFromOtlp([]*otlplogs.ResourceLogs{ + })).LogRecordCount()) + assert.EqualValues(t, 2, LogsFromInternalRep(internal.LogsFromOtlp([]*otlplogs.ResourceLogs{ { InstrumentationLibraryLogs: []*otlplogs.InstrumentationLibraryLogs{ { @@ -60,12 +61,12 @@ func TestLogRecordCountWithNils(t *testing.T) { }, }, }, - }).LogRecordCount()) + })).LogRecordCount()) } func TestToFromLogProto(t *testing.T) { otlp := []*otlplogs.ResourceLogs(nil) - td := LogsFromOtlp(otlp) + td := LogsFromInternalRep(internal.LogsFromOtlp(otlp)) assert.EqualValues(t, NewLogs(), td) - assert.EqualValues(t, otlp, LogsToOtlp(td)) + assert.EqualValues(t, otlp, *td.orig) } diff --git a/exporter/fileexporter/file_exporter.go b/exporter/fileexporter/file_exporter.go index c479c909e04..a6edfc0d2c1 100644 --- a/exporter/fileexporter/file_exporter.go +++ b/exporter/fileexporter/file_exporter.go @@ -24,6 +24,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/consumer/pdata" + "go.opentelemetry.io/collector/internal" otlplogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/logs/v1" otlpmetrics "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/metrics/v1" otlptrace "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/trace/v1" @@ -55,7 +56,7 @@ func (e *fileExporter) ConsumeMetrics(_ context.Context, md pdata.Metrics) error func (e *fileExporter) ConsumeLogs(_ context.Context, ld pdata.Logs) error { request := otlplogs.ExportLogsServiceRequest{ - ResourceLogs: pdata.LogsToOtlp(ld), + ResourceLogs: internal.LogsToOtlp(ld.InternalRep()), } return exportMessageAsLine(e, &request) } diff --git a/exporter/fileexporter/file_exporter_test.go b/exporter/fileexporter/file_exporter_test.go index e317954e109..262a103a763 100644 --- a/exporter/fileexporter/file_exporter_test.go +++ b/exporter/fileexporter/file_exporter_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/consumer/pdata" + "go.opentelemetry.io/collector/internal" collectorlogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/logs/v1" collectormetrics "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/metrics/v1" collectortrace "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/trace/v1" @@ -118,7 +119,7 @@ func TestFileLogsExporterNoErrors(t *testing.T) { }, }, } - assert.NoError(t, exporter.ConsumeLogs(context.Background(), pdata.LogsFromOtlp(ld))) + assert.NoError(t, exporter.ConsumeLogs(context.Background(), pdata.LogsFromInternalRep(internal.LogsFromOtlp(ld)))) assert.NoError(t, exporter.Shutdown(context.Background())) var unmarshaler = &jsonpb.Unmarshaler{} @@ -209,7 +210,7 @@ func TestFileLogsExporterErrors(t *testing.T) { exporter := &fileExporter{file: mf} require.NotNil(t, exporter) - assert.Error(t, exporter.ConsumeLogs(context.Background(), pdata.LogsFromOtlp(ld))) + assert.Error(t, exporter.ConsumeLogs(context.Background(), pdata.LogsFromInternalRep(internal.LogsFromOtlp(ld)))) assert.NoError(t, exporter.Shutdown(context.Background())) }) } diff --git a/exporter/otlpexporter/otlp.go b/exporter/otlpexporter/otlp.go index 962bd0cf986..d9bca44bb9b 100644 --- a/exporter/otlpexporter/otlp.go +++ b/exporter/otlpexporter/otlp.go @@ -30,6 +30,7 @@ import ( "go.opentelemetry.io/collector/consumer/consumererror" "go.opentelemetry.io/collector/consumer/pdata" "go.opentelemetry.io/collector/exporter/exporterhelper" + "go.opentelemetry.io/collector/internal" otlplogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/logs/v1" otlpmetrics "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/metrics/v1" otlptrace "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/trace/v1" @@ -101,7 +102,7 @@ func (e *exporterImp) pushMetricsData(ctx context.Context, md pdata.Metrics) (in func (e *exporterImp) pushLogData(ctx context.Context, logs pdata.Logs) (int, error) { request := &otlplogs.ExportLogsServiceRequest{ - ResourceLogs: pdata.LogsToOtlp(logs), + ResourceLogs: internal.LogsToOtlp(logs.InternalRep()), } err := e.w.exportLogs(ctx, request) diff --git a/internal/data/testdata/log.go b/internal/data/testdata/log.go index 961f187e474..b7f7c607cc2 100644 --- a/internal/data/testdata/log.go +++ b/internal/data/testdata/log.go @@ -20,6 +20,7 @@ import ( otlplogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/logs/v1" "go.opentelemetry.io/collector/consumer/pdata" + "go.opentelemetry.io/collector/internal" otlpcommon "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/common/v1" ) @@ -54,7 +55,7 @@ func generateLogOtlpOneEmptyResourceLogs() []*otlplogs.ResourceLogs { } func GenerateLogDataOneEmptyOneNilResourceLogs() pdata.Logs { - return pdata.LogsFromOtlp(generateLogOtlpOneEmptyOneNilResourceLogs()) + return pdata.LogsFromInternalRep(internal.LogsFromOtlp(generateLogOtlpOneEmptyOneNilResourceLogs())) } @@ -104,7 +105,7 @@ func generateLogOtlpOneEmptyLogs() []*otlplogs.ResourceLogs { } func GenerateLogDataOneEmptyOneNilLogRecord() pdata.Logs { - return pdata.LogsFromOtlp(generateLogOtlpOneEmptyOneNilLogRecord()) + return pdata.LogsFromInternalRep(internal.LogsFromOtlp(generateLogOtlpOneEmptyOneNilLogRecord())) } func generateLogOtlpOneEmptyOneNilLogRecord() []*otlplogs.ResourceLogs { @@ -173,7 +174,7 @@ func generateLogOtlpOneLog() []*otlplogs.ResourceLogs { } func GenerateLogDataOneLogOneNil() pdata.Logs { - return pdata.LogsFromOtlp(generateLogOtlpOneLogOneNil()) + return pdata.LogsFromInternalRep(internal.LogsFromOtlp(generateLogOtlpOneLogOneNil())) } func generateLogOtlpOneLogOneNil() []*otlplogs.ResourceLogs { diff --git a/internal/data/testdata/log_test.go b/internal/data/testdata/log_test.go index 972c5e187fa..5b2e0db3bcb 100644 --- a/internal/data/testdata/log_test.go +++ b/internal/data/testdata/log_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "go.opentelemetry.io/collector/consumer/pdata" + "go.opentelemetry.io/collector/internal" otlplogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/logs/v1" ) @@ -96,9 +97,9 @@ func TestToFromOtlpLog(t *testing.T) { for i := range allTestCases { test := allTestCases[i] t.Run(test.name, func(t *testing.T) { - ld := pdata.LogsFromOtlp(test.otlp) + ld := pdata.LogsFromInternalRep(internal.LogsFromOtlp(test.otlp)) assert.EqualValues(t, test.ld, ld) - otlp := pdata.LogsToOtlp(ld) + otlp := internal.LogsToOtlp(ld.InternalRep()) assert.EqualValues(t, test.otlp, otlp) }) } diff --git a/internal/empty_test.go b/internal/empty_test.go new file mode 100644 index 00000000000..9814ff84018 --- /dev/null +++ b/internal/empty_test.go @@ -0,0 +1,15 @@ +// 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 internal diff --git a/internal/otlp_wrapper.go b/internal/otlp_wrapper.go new file mode 100644 index 00000000000..5b1fa36bd09 --- /dev/null +++ b/internal/otlp_wrapper.go @@ -0,0 +1,32 @@ +// 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 internal + +import otlplogs "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/logs/v1" + +// OtlpLogsWrapper is an intermediary struct that is declared in an internal package +// as a way to prevent certain functions of pdata.Logs data type to be callable by +// any code outside of this module. +type OtlpLogsWrapper struct { + Orig *[]*otlplogs.ResourceLogs +} + +func LogsToOtlp(l OtlpLogsWrapper) []*otlplogs.ResourceLogs { + return *l.Orig +} + +func LogsFromOtlp(logs []*otlplogs.ResourceLogs) OtlpLogsWrapper { + return OtlpLogsWrapper{Orig: &logs} +} diff --git a/receiver/otlpreceiver/logs/otlp.go b/receiver/otlpreceiver/logs/otlp.go index 9f4bf6862e8..7b5c6fe7047 100644 --- a/receiver/otlpreceiver/logs/otlp.go +++ b/receiver/otlpreceiver/logs/otlp.go @@ -20,6 +20,7 @@ import ( "go.opentelemetry.io/collector/client" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/consumer/pdata" + "go.opentelemetry.io/collector/internal" collectorlog "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/logs/v1" "go.opentelemetry.io/collector/obsreport" ) @@ -53,7 +54,7 @@ func (r *Receiver) Export(ctx context.Context, req *collectorlog.ExportLogsServi // We need to ensure that it propagates the receiver name as a tag ctxWithReceiverName := obsreport.ReceiverContext(ctx, r.instanceName, receiverTransport, receiverTagValue) - ld := pdata.LogsFromOtlp(req.ResourceLogs) + ld := pdata.LogsFromInternalRep(internal.LogsFromOtlp(req.ResourceLogs)) err := r.sendToNextConsumer(ctxWithReceiverName, ld) if err != nil { return nil, err diff --git a/receiver/otlpreceiver/logs/otlp_test.go b/receiver/otlpreceiver/logs/otlp_test.go index 5838b64202d..43340b744b9 100644 --- a/receiver/otlpreceiver/logs/otlp_test.go +++ b/receiver/otlpreceiver/logs/otlp_test.go @@ -28,6 +28,7 @@ import ( "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/consumer/pdata" "go.opentelemetry.io/collector/exporter/exportertest" + "go.opentelemetry.io/collector/internal" collectorlog "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/collector/logs/v1" otlplog "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/logs/v1" "go.opentelemetry.io/collector/obsreport" @@ -77,7 +78,7 @@ func TestExport(t *testing.T) { // Keep log data to compare the test result against it // Clone needed because OTLP proto XXX_ fields are altered in the GRPC downstream - traceData := pdata.LogsFromOtlp(resourceLogs).Clone() + traceData := pdata.LogsFromInternalRep(internal.LogsFromOtlp(resourceLogs)).Clone() req := &collectorlog.ExportLogsServiceRequest{ ResourceLogs: resourceLogs,