Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make DataType a separate type within Component #10069

Closed
28 changes: 28 additions & 0 deletions .chloggen/datatype-type-interface.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: component

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Change Type to an interface, introduce the new types ComponentType and DataType now implement it."

# One or more tracking issues or pull requests related to the change
issues: [9429]

# (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: |
Creates ComponentType and DataType - ComponentType represents the names of components and DataType is an enum for Traces,Logs, and Metrics (and future signal types!).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComponentType represents the names of components

I think we need to be very precise here not to overload the words type or name. Currently, for components we use ID to refer to type[/name]. So when you say "names of components", I think this should actually say Type of component. It may be helpful to give a couple clear examples for users as well. "The type of component, e.g. otlp, filelog"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great feedback - my use of the word name didn't account for name actually being embedded within ID


NewType will now automatically create a DataType instead of ComponentType for the strings "traces", "logs", and "metrics". In addition, ID's zero value now contains a null value for Type - since it is now an interface."

# 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: ['api']
63 changes: 48 additions & 15 deletions component/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package component // import "go.opentelemetry.io/collector/component"

import (
"encoding"
"fmt"
"reflect"
"regexp"
Expand Down Expand Up @@ -109,18 +110,26 @@
return nil
}

// Type is the component type as it is used in the config.
type Type struct {
// Type represents the names of receivers (otlp, filelog, etc),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

// processors (batch, memory_limit, etc), or exporters (debug, rabbitmq)
// It also includes the DataType - things like "metrics", "traces", "logs"
type Type interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to keep ID basically the same, we make Type an interface and implement it with ComponentType and DataType

ankitpatel96 marked this conversation as resolved.
Show resolved Hide resolved
fmt.Stringer
encoding.TextMarshaler
}

// ComponentType is the component type as it is used in the config.
type ComponentType struct { //revive:disable-line:exported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the linter directive needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter doesn't like it when type FooBar is in a package Foo. It thinks Foo/FooBar is annoying to pronounce. I think in this case having component/ComponentType is a good exemption.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the linter here. The difference between component.Type and component.ComponentType is very not obvious at a glance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name string
}

// String returns the string representation of the type.
func (t Type) String() string {
func (t ComponentType) String() string {
return t.name
}

// MarshalText marshals returns the Type name.
func (t Type) MarshalText() ([]byte, error) {
func (t ComponentType) MarshalText() ([]byte, error) {

Check warning on line 132 in component/config.go

View check run for this annotation

Codecov / codecov/patch

component/config.go#L132

Added line #L132 was not covered by tests
return []byte(t.name), nil
}

Expand All @@ -135,14 +144,18 @@
// - have at least one character,
// - start with an ASCII alphabetic character and
// - can only contain ASCII alphanumeric characters and '_'.
// If the type string is a supported DataType, one is returned. Otherwise, a ComponentType is returned.
func NewType(ty string) (Type, error) {
if len(ty) == 0 {
return Type{}, fmt.Errorf("id must not be empty")
return ComponentType{}, fmt.Errorf("id must not be empty")
}
if !typeRegexp.MatchString(ty) {
return Type{}, fmt.Errorf("invalid character(s) in type %q", ty)
return ComponentType{}, fmt.Errorf("invalid character(s) in type %q", ty)
}
return Type{name: ty}, nil
if dataType, ok := DataTypeFromSignal(ty); ok {
return dataType, nil
}
return ComponentType{name: ty}, nil
}

// MustNewType creates a type. It panics if the type is invalid.
Expand All @@ -160,20 +173,40 @@

// DataType is a special Type that represents the data types supported by the collector. We currently support
// collecting metrics, traces and logs, this can expand in the future.
type DataType = Type

func mustNewDataType(strType string) DataType {
return MustNewType(strType)
}
type DataType string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use a struct here to disallow users to create new DataType's

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I understand that's a risk. I've tried to mitigate it with the logic in pipelines/config.go that make sure its a member of signalNameToDataType.

Do you think its still worth wrapping it in a struct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to rework all of this, it would make sense to me to move data type into a different package, since things other than components can be types.


// Currently supported data types. Add new data types here when new types are supported in the future.
var (
// DataTypeTraces is the data type tag for traces.
DataTypeTraces = mustNewDataType("traces")
DataTypeTraces DataType = "traces"

// DataTypeMetrics is the data type tag for metrics.
DataTypeMetrics = mustNewDataType("metrics")
DataTypeMetrics DataType = "metrics"

// DataTypeLogs is the data type tag for logs.
DataTypeLogs = mustNewDataType("logs")
DataTypeLogs DataType = "logs"

signalNameToDataType = map[string]DataType{
"logs": DataTypeLogs,
"metrics": DataTypeMetrics,
"traces": DataTypeTraces,
}
)

func (dt DataType) String() string {
return string(dt)

Check warning on line 197 in component/config.go

View check run for this annotation

Codecov / codecov/patch

component/config.go#L196-L197

Added lines #L196 - L197 were not covered by tests
}
func (dt DataType) MarshalText() (text []byte, err error) {
return []byte(dt), nil

Check warning on line 200 in component/config.go

View check run for this annotation

Codecov / codecov/patch

component/config.go#L199-L200

Added lines #L199 - L200 were not covered by tests
}

// DataTypeFromSignal takes in a string of a datatype, and returns a DataType and a bool.
// The bool is set to true if the string corresponds to a supported DataType, else it is False.
// if there is a matching DataType, it returns the matching DataType enum. If not, it returns an empty string.
func DataTypeFromSignal(s string) (DataType, bool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would instead implement UnmarshalText for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea!

val, ok := signalNameToDataType[s]
if !ok {
return "", ok
}
return val, ok
}
24 changes: 23 additions & 1 deletion component/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
"go.opentelemetry.io/collector/confmap"
)

var _ fmt.Stringer = Type{}
var _ fmt.Stringer = ComponentType{}

type configChildStruct struct {
Child errConfig
Expand Down Expand Up @@ -423,6 +423,18 @@ func TestNewType(t *testing.T) {
}
}

func TestNewTypeDataType(t *testing.T) {
ty, err := NewType("logs")
require.NoError(t, err)
assert.Equal(t, ty, DataTypeLogs)

// hopefully this reminds us to update DataType when profiles get included
ty, err = NewType("profiles")
require.NoError(t, err)
assert.Equal(t, ty.(ComponentType).name, "profiles")

}

type configWithEmbeddedStruct struct {
String string `mapstructure:"string"`
Num int `mapstructure:"num"`
Expand All @@ -447,3 +459,13 @@ func TestStructWithEmbeddedUnmarshaling(t *testing.T) {
assert.Equal(t, "foo", tc.String)
assert.Equal(t, 123, tc.Num)
}

func TestDataTypeFromSignal(t *testing.T) {
dt, ok := DataTypeFromSignal("logs")
assert.Equal(t, ok, true)
assert.Equal(t, DataTypeLogs, dt)

dt, ok = DataTypeFromSignal("asdf")
assert.Equal(t, ok, false)
assert.Equal(t, DataType(""), dt)
}
8 changes: 5 additions & 3 deletions component/identifiable.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ import (
const typeAndNameSeparator = "/"

// ID represents the identity for a component. It combines two values:
// * type - the Type of the component.
// * name - the name of that component.
// The component ID (combination type + name) is unique for a given component.Kind.
// * typeVal - the Type of the component.
// * nameVal - the name of that component. This is optional and can be an empty string.
// The Component ID (combination type + name) is unique for a given component.Kind.
// Component IDs are defined in configuration by type[/name] - for examples [traces/1] or [oltp/blah]
type ID struct {
typeVal Type `mapstructure:"-"`
nameVal string `mapstructure:"-"`
Expand Down Expand Up @@ -88,6 +89,7 @@ func (id *ID) UnmarshalText(text []byte) error {
if id.typeVal, err = NewType(typeStr); err != nil {
return fmt.Errorf("in %q id: %w", idStr, err)
}

id.nameVal = nameStr

return nil
Expand Down
16 changes: 8 additions & 8 deletions exporter/exporterhelper/batch_sender_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func TestBatchSender_Disabled(t *testing.T) {
cfg := exporterbatcher.NewDefaultConfig()
cfg.Enabled = false
cfg.MaxSizeItems = 5
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender,
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender,
WithBatcher(cfg, WithRequestBatchFuncs(fakeBatchMergeFunc, fakeBatchMergeSplitFunc)))
require.NotNil(t, be)
require.NoError(t, err)
Expand Down Expand Up @@ -257,7 +257,7 @@ func TestBatchSender_InvalidMergeSplitFunc(t *testing.T) {
}

func TestBatchSender_PostShutdown(t *testing.T) {
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender,
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender,
WithBatcher(exporterbatcher.NewDefaultConfig(), WithRequestBatchFuncs(fakeBatchMergeFunc,
fakeBatchMergeSplitFunc)))
require.NotNil(t, be)
Expand All @@ -275,7 +275,7 @@ func TestBatchSender_PostShutdown(t *testing.T) {
func TestBatchSender_ConcurrencyLimitReached(t *testing.T) {
qCfg := exporterqueue.NewDefaultConfig()
qCfg.NumConsumers = 2
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender,
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender,
WithBatcher(exporterbatcher.NewDefaultConfig(), WithRequestBatchFuncs(fakeBatchMergeFunc, fakeBatchMergeSplitFunc)),
WithRequestQueue(qCfg, exporterqueue.NewMemoryQueueFactory[Request]()))
require.NotNil(t, be)
Expand All @@ -299,7 +299,7 @@ func TestBatchSender_ConcurrencyLimitReached(t *testing.T) {
func TestBatchSender_BatchBlocking(t *testing.T) {
bCfg := exporterbatcher.NewDefaultConfig()
bCfg.MinSizeItems = 3
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender,
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender,
WithBatcher(bCfg, WithRequestBatchFuncs(fakeBatchMergeFunc, fakeBatchMergeSplitFunc)))
require.NotNil(t, be)
require.NoError(t, err)
Expand Down Expand Up @@ -329,7 +329,7 @@ func TestBatchSender_BatchBlocking(t *testing.T) {
func TestBatchSender_BatchCancelled(t *testing.T) {
bCfg := exporterbatcher.NewDefaultConfig()
bCfg.MinSizeItems = 2
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender,
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender,
WithBatcher(bCfg, WithRequestBatchFuncs(fakeBatchMergeFunc, fakeBatchMergeSplitFunc)))
require.NotNil(t, be)
require.NoError(t, err)
Expand Down Expand Up @@ -364,7 +364,7 @@ func TestBatchSender_BatchCancelled(t *testing.T) {
func TestBatchSender_DrainActiveRequests(t *testing.T) {
bCfg := exporterbatcher.NewDefaultConfig()
bCfg.MinSizeItems = 2
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender,
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender,
WithBatcher(bCfg, WithRequestBatchFuncs(fakeBatchMergeFunc, fakeBatchMergeSplitFunc)))
require.NotNil(t, be)
require.NoError(t, err)
Expand Down Expand Up @@ -427,7 +427,7 @@ func TestBatchSender_WithBatcherOption(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender, tt.opts...)
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender, tt.opts...)
if tt.expectedErr {
assert.Nil(t, be)
assert.Error(t, err)
Expand All @@ -440,7 +440,7 @@ func TestBatchSender_WithBatcherOption(t *testing.T) {
}

func queueBatchExporter(t *testing.T, batchOption Option) *baseExporter {
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender, batchOption,
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender, batchOption,
WithRequestQueue(exporterqueue.NewDefaultConfig(), exporterqueue.NewMemoryQueueFactory[Request]()))
require.NotNil(t, be)
require.NoError(t, err)
Expand Down
13 changes: 7 additions & 6 deletions exporter/exporterhelper/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

var (
defaultType = component.MustNewType("test")
defaultDataType = component.DataTypeMetrics
ankitpatel96 marked this conversation as resolved.
Show resolved Hide resolved
defaultID = component.NewID(defaultType)
defaultSettings = func() exporter.CreateSettings {
set := exportertest.NewNopCreateSettings()
Expand All @@ -37,7 +38,7 @@ func newNoopObsrepSender(*ObsReport) requestSender {
}

func TestBaseExporter(t *testing.T) {
be, err := newBaseExporter(defaultSettings, defaultType, newNoopObsrepSender)
be, err := newBaseExporter(defaultSettings, defaultDataType, newNoopObsrepSender)
require.NoError(t, err)
require.NoError(t, be.Start(context.Background(), componenttest.NewNopHost()))
require.NoError(t, be.Shutdown(context.Background()))
Expand All @@ -46,7 +47,7 @@ func TestBaseExporter(t *testing.T) {
func TestBaseExporterWithOptions(t *testing.T) {
want := errors.New("my error")
be, err := newBaseExporter(
defaultSettings, defaultType, newNoopObsrepSender,
defaultSettings, defaultDataType, newNoopObsrepSender,
WithStart(func(context.Context, component.Host) error { return want }),
WithShutdown(func(context.Context) error { return want }),
WithTimeout(NewDefaultTimeoutSettings()),
Expand All @@ -66,16 +67,16 @@ func checkStatus(t *testing.T, sd sdktrace.ReadOnlySpan, err error) {
}

func TestQueueOptionsWithRequestExporter(t *testing.T) {
bs, err := newBaseExporter(exportertest.NewNopCreateSettings(), defaultType, newNoopObsrepSender,
bs, err := newBaseExporter(exportertest.NewNopCreateSettings(), defaultDataType, newNoopObsrepSender,
WithRetry(configretry.NewDefaultBackOffConfig()))
require.Nil(t, err)
require.Nil(t, bs.marshaler)
require.Nil(t, bs.unmarshaler)
_, err = newBaseExporter(exportertest.NewNopCreateSettings(), defaultType, newNoopObsrepSender,
_, err = newBaseExporter(exportertest.NewNopCreateSettings(), defaultDataType, newNoopObsrepSender,
WithRetry(configretry.NewDefaultBackOffConfig()), WithQueue(NewDefaultQueueSettings()))
require.Error(t, err)

_, err = newBaseExporter(exportertest.NewNopCreateSettings(), defaultType, newNoopObsrepSender,
_, err = newBaseExporter(exportertest.NewNopCreateSettings(), defaultDataType, newNoopObsrepSender,
withMarshaler(mockRequestMarshaler), withUnmarshaler(mockRequestUnmarshaler(&mockRequest{})),
WithRetry(configretry.NewDefaultBackOffConfig()),
WithRequestQueue(exporterqueue.NewDefaultConfig(), exporterqueue.NewMemoryQueueFactory[Request]()))
Expand All @@ -88,7 +89,7 @@ func TestBaseExporterLogging(t *testing.T) {
set.Logger = zap.New(logger)
rCfg := configretry.NewDefaultBackOffConfig()
rCfg.Enabled = false
bs, err := newBaseExporter(set, defaultType, newNoopObsrepSender, WithRetry(rCfg))
bs, err := newBaseExporter(set, defaultDataType, newNoopObsrepSender, WithRetry(rCfg))
require.Nil(t, err)
sendErr := bs.send(context.Background(), newErrorRequest())
require.Error(t, sendErr)
Expand Down
Loading
Loading