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.

Sign up for GitHub

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

Jump to bottom

Make DataType a separate type within Component #10222

Closed
wants to merge 39 commits into from
Closed

Make DataType a separate type within Component #10222

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
1329377
make a new id type with datatype and use it
ankitpatel96 Apr 27, 2024
4a0e0fa
PipelineID
ankitpatel96 May 24, 2024
d782e7d
prune methods
ankitpatel96 May 24, 2024
aa519a7
methods
ankitpatel96 May 24, 2024
1904b6d
remove unecessary constructor
ankitpatel96 May 24, 2024
f80c5ec
delete comment
ankitpatel96 May 24, 2024
91bf4f0
mass replace id constructors
ankitpatel96 May 24, 2024
e5c7f5d
switch tests to pipelineid
ankitpatel96 May 24, 2024
e401781
change datagen
ankitpatel96 May 25, 2024
ab1811d
cleanup changes
ankitpatel96 May 25, 2024
73bbec2
fix rebase
ankitpatel96 May 25, 2024
c9180b7
private method
ankitpatel96 May 28, 2024
42dae7d
move pipeline id to new file
ankitpatel96 May 28, 2024
eaa4e20
test coverage
ankitpatel96 May 28, 2024
cb9e83b
copyright
ankitpatel96 May 28, 2024
d7422bd
porto
ankitpatel96 May 28, 2024
66d375f
lint
ankitpatel96 May 28, 2024
631568c
chlog
ankitpatel96 May 28, 2024
77778a9
unit test
ankitpatel96 May 28, 2024
4b548a7
Merge branch 'main' into datatype-newid
ankitpatel96 Jun 10, 2024
149cb87
fix
ankitpatel96 Jun 10, 2024
5aa24ba
more merge fixups
ankitpatel96 Jun 10, 2024
5b96c29
move instanceid
ankitpatel96 Jun 10, 2024
304d0b0
go.mod tidy
ankitpatel96 Jun 10, 2024
6a94f32
separator is public
ankitpatel96 Jun 12, 2024
b86c1e4
datatype unmarshal
ankitpatel96 Jun 12, 2024
777bb20
new module
ankitpatel96 Jun 12, 2024
a43fa35
go mod updates
ankitpatel96 Jun 12, 2024
929f67a
rename pipeline id
ankitpatel96 Jun 12, 2024
2c1a00b
instance id move
ankitpatel96 Jun 12, 2024
4aec05c
instanceid go mod
ankitpatel96 Jun 12, 2024
dac83e3
crosslink
ankitpatel96 Jun 12, 2024
a302ca1
trim tags
ankitpatel96 Jun 12, 2024
3676ca0
rename some methods
ankitpatel96 Jun 12, 2024
22b2539
chloggen
ankitpatel96 Jun 12, 2024
1475a25
quote the bad datatype
ankitpatel96 Jun 24, 2024
534db04
update component tests
ankitpatel96 Jun 25, 2024
3d82883
add to changelog
ankitpatel96 Jun 25, 2024
c1252e4
license
ankitpatel96 Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .chloggen/datatype-newid.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: 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: "Changes DataType to be an enum of Otel signals. Creates a new struct PipelineID - made up of a DataType and an optional string name. This PipelineID replaces the previous use of component.ID to identify Pipelines."

# 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: ""

# 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']
18 changes: 17 additions & 1 deletion cmd/mdatagen/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ require (
go.opentelemetry.io/otel/sdk v1.27.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
golang.org/x/net v0.25.0 // indirect
golang.org/x/sys v0.20.0 // indirect
golang.org/x/sys v0.21.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
google.golang.org/grpc v1.64.0 // indirect
google.golang.org/protobuf v1.34.1 // indirect
Expand Down Expand Up @@ -85,3 +85,19 @@ retract (
v0.76.1
v0.65.0
)

replace go.opentelemetry.io/collector/extension/zpagesextension => ../../extension/zpagesextension

replace go.opentelemetry.io/collector/connector => ../../connector

replace go.opentelemetry.io/collector/processor => ../../processor

replace go.opentelemetry.io/collector/exporter => ../../exporter

replace go.opentelemetry.io/collector/config/confignet => ../../config/confignet

replace go.opentelemetry.io/collector/config/configretry => ../../config/configretry

replace go.opentelemetry.io/collector/service => ../../service

replace go.opentelemetry.io/collector/extension => ../../extension
4 changes: 2 additions & 2 deletions cmd/mdatagen/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 9 additions & 9 deletions cmd/mdatagen/templates/component_test.go.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "logs_to_logs",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewLogsRouter(map[component.ID]consumer.Logs{component.NewID(component.DataTypeLogs): consumertest.NewNop()})
router := connector.NewLogsRouter(map[component.PipelineID]consumer.Logs{component.NewPipelineID(component.DataTypeLogs): consumertest.NewNop()})
return factory.CreateLogsToLogs(ctx, set, cfg, router)
},
},
Expand All @@ -381,7 +381,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "logs_to_metrics",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewMetricsRouter(map[component.ID]consumer.Metrics{component.NewID(component.DataTypeMetrics): consumertest.NewNop()})
router := connector.NewMetricsRouter(map[component.PipelineID]consumer.Metrics{component.NewPipelineID(component.DataTypeMetrics): consumertest.NewNop()})
return factory.CreateLogsToMetrics(ctx, set, cfg, router)
},
},
Expand All @@ -390,7 +390,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "logs_to_traces",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewTracesRouter(map[component.ID]consumer.Traces{component.NewID(component.DataTypeTraces): consumertest.NewNop()})
router := connector.NewTracesRouter(map[component.PipelineID]consumer.Traces{component.NewPipelineID(component.DataTypeTraces): consumertest.NewNop()})
return factory.CreateLogsToTraces(ctx, set, cfg, router)
},
},
Expand All @@ -399,7 +399,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "metrics_to_logs",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewLogsRouter(map[component.ID]consumer.Logs{component.NewID(component.DataTypeLogs): consumertest.NewNop()})
router := connector.NewLogsRouter(map[component.PipelineID]consumer.Logs{component.NewPipelineID(component.DataTypeLogs): consumertest.NewNop()})
return factory.CreateMetricsToLogs(ctx, set, cfg, router)
},
},
Expand All @@ -408,7 +408,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "metrics_to_metrics",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewMetricsRouter(map[component.ID]consumer.Metrics{component.NewID(component.DataTypeMetrics): consumertest.NewNop()})
router := connector.NewMetricsRouter(map[component.PipelineID]consumer.Metrics{component.NewPipelineID(component.DataTypeMetrics): consumertest.NewNop()})
return factory.CreateMetricsToMetrics(ctx, set, cfg, router)
},
},
Expand All @@ -417,7 +417,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "metrics_to_traces",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewTracesRouter(map[component.ID]consumer.Traces{component.NewID(component.DataTypeTraces): consumertest.NewNop()})
router := connector.NewTracesRouter(map[component.PipelineID]consumer.Traces{component.NewPipelineID(component.DataTypeTraces): consumertest.NewNop()})
return factory.CreateMetricsToTraces(ctx, set, cfg, router)
},
},
Expand All @@ -426,7 +426,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "traces_to_logs",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewLogsRouter(map[component.ID]consumer.Logs{component.NewID(component.DataTypeLogs): consumertest.NewNop()})
router := connector.NewLogsRouter(map[component.PipelineID]consumer.Logs{component.NewPipelineID(component.DataTypeLogs): consumertest.NewNop()})
return factory.CreateTracesToLogs(ctx, set, cfg, router)
},
},
Expand All @@ -435,7 +435,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "traces_to_metrics",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewMetricsRouter(map[component.ID]consumer.Metrics{component.NewID(component.DataTypeMetrics): consumertest.NewNop()})
router := connector.NewMetricsRouter(map[component.PipelineID]consumer.Metrics{component.NewPipelineID(component.DataTypeMetrics): consumertest.NewNop()})
return factory.CreateTracesToMetrics(ctx, set, cfg, router)
},
},
Expand All @@ -444,7 +444,7 @@ func TestComponentLifecycle(t *testing.T) {
{
name: "traces_to_traces",
createFn: func(ctx context.Context, set connector.Settings, cfg component.Config) (component.Component, error) {
router := connector.NewTracesRouter(map[component.ID]consumer.Traces{component.NewID(component.DataTypeTraces): consumertest.NewNop()})
router := connector.NewTracesRouter(map[component.PipelineID]consumer.Traces{component.NewPipelineID(component.DataTypeTraces): consumertest.NewNop()})
return factory.CreateTracesToTraces(ctx, set, cfg, router)
},
},
Expand Down
7 changes: 0 additions & 7 deletions component/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,3 @@ type CreateDefaultConfigFunc func() Config
func (f CreateDefaultConfigFunc) CreateDefaultConfig() Config {
return f()
}

// InstanceID uniquely identifies a component instance
type InstanceID struct {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a

// Deprecated use [<ref to new type>]
type InstanceID = <ref to new type>

here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I? That would make component dependent on pipelines... which is most definitely circular

Copy link
Member

Choose a reason for hiding this comment

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

We can use an internal module and make this and the actual type aliases of a symbol in that internal module until we remove it from component:

// package internal

type InstanceID struct { /* actual definition goes here *? } 
// package component

// Deprecated: [v0.103.0] use pipelines.InstanceID
type InstanceID = internal.InstanceID
// package pipelines

type InstanceID = internal.InstanceID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is going to work -

Pipeline depends on internal, which depends on pipeline since InstanceID depends on pipeline.ID

If I move pipeline.ID into this internal package, we solve that problem but then the component.InstanceID shim doesn't work - that means component imports internal and then internal imports component (InstanceID contains component.ID and component.Kind).

Open to any ideas to fixing this..

Side note - this internal package has to be collector/internal/ since it needs to be accessible by collector/component and collector/pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

A solution here would be to also move component.ID and component.Kind to internal and make shims also for these two on component. It sounds like a pain, so let's first discuss the rest of the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right that would probably work... but I do question whether its worth it for stability concerns. Moving component.ID to internal/ would make component dependent on the root collector module, thus adding that dependency to almost every other module.

I don't think the blast radius of this change is especially large for contrib and I think, in this case, it makes sense to keep this change as is and not try to provide backwards compatible import shims.

ID ID
Kind Kind
PipelineIDs map[ID]struct{}
}
41 changes: 32 additions & 9 deletions component/config.go
View file
Open in desktop
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 (
"errors"
"fmt"
"reflect"
"regexp"
Expand Down Expand Up @@ -97,6 +98,8 @@ func callValidateIfPossible(v reflect.Value) error {
}

// Type is the component type as it is used in the config.
// Examples of a Type include the names of receivers (otlp, filelog, etc),
// processors (batch, memory_limit, etc), or exporters (debug, rabbitmq)
type Type struct {
name string
}
Expand Down Expand Up @@ -147,20 +150,40 @@ func MustNewType(strType string) Type {

// 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

// Currently supported data types. Add new data types here when new types are supported in the future.
var (
const (
// 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"
)

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

func (dt DataType) MarshalText() (text []byte, err error) {
return []byte(dt), nil
}

func newDataType(ty string) (DataType, error) {
if len(ty) == 0 {
return "", errors.New("id must not be empty")
}
switch ty {
case "metrics":
return DataTypeMetrics, nil
case "logs":
return DataTypeLogs, nil
case "traces":
return DataTypeTraces, nil
default:
return "", fmt.Errorf("invalid data type %s", ty)
}
}
ankitpatel96 marked this conversation as resolved.
Show resolved Hide resolved
33 changes: 33 additions & 0 deletions component/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,3 +420,36 @@ func TestNewType(t *testing.T) {
})
}
}

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

// hopefully this reminds us to update DataType when profiles get included
ty, err = newDataType("profiles")
assert.Equal(t, DataType(""), ty)
assert.Equal(t, err, errors.New("invalid data type profiles"))
}

func TestDataTypeStringMarshal(t *testing.T) {
assert.Equal(t, "metrics", DataTypeMetrics.String())
text, err := DataTypeMetrics.MarshalText()
assert.Equal(t, []byte("metrics"), text)
assert.Nil(t, err)
}

func TestDataTypeFromSignal(t *testing.T) {
dt, err := newDataType("metrics")
assert.Nil(t, err)
assert.Equal(t, DataTypeMetrics, dt)

dt, err = newDataType("traces")
assert.Equal(t, DataTypeTraces, dt)
assert.Nil(t, err)

dt, err = newDataType("")
assert.Equal(t, errors.New("id must not be empty"), err)
assert.Equal(t, DataType(""), dt)

}
6 changes: 3 additions & 3 deletions component/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ require (
github.com/gogo/protobuf v1.3.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/prometheus/procfs v0.15.0 // indirect
golang.org/x/net v0.24.0 // indirect
golang.org/x/net v0.25.0 // indirect
golang.org/x/sys v0.20.0 // indirect
golang.org/x/text v0.14.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 // indirect
golang.org/x/text v0.15.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
google.golang.org/grpc v1.64.0 // indirect
google.golang.org/protobuf v1.34.1 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
Expand Down
12 changes: 6 additions & 6 deletions component/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion component/identifiable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ 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.
// * name - the name of that component. This can be an empty string.
// Components are defined in configuration by type[/name] - for examples [traces/1] or [oltp/blah]
// The component ID (combination type + name) is unique for a given component.Kind.
type ID struct {
typeVal Type `mapstructure:"-"`
Expand Down
Loading
Loading