-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 #10222
Changes from all commits
1329377
4a0e0fa
d782e7d
aa519a7
1904b6d
f80c5ec
91bf4f0
e5c7f5d
e401781
ab1811d
73bbec2
c9180b7
42dae7d
eaa4e20
cb9e83b
d7422bd
66d375f
631568c
77778a9
4b548a7
149cb87
5aa24ba
5b96c29
304d0b0
6a94f32
b86c1e4
777bb20
a43fa35
929f67a
2c1a00b
4aec05c
dac83e3
a302ca1
3676ca0
22b2539
1475a25
534db04
3d82883
c1252e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: breaking | ||
|
||
# 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 module pipeline to centralize types related to pipelines. Creates a new struct pipeline.ID - made up of a DataType and an optional string name. This pipeline.ID replaces the previous use of component.ID to identify Pipelines. Moves InstanceID to pipeline to break import cycles." | ||
|
||
# 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: | | ||
Related API changes: | ||
connector.LogsRouterAndConsumer, connector.MetricsRouterAndConsumer, and connector.TracesRouterAndConsumer `Consumer` methods now accepts pipeline.ID instead of component.ID. Their PipelineIDs function return pipeline.ID instead of component.ID. | ||
NewStatusWatcherExtensionFactory now accepts a pipeline.InstanceID instead of component.InstanceID. | ||
The generated component tests now use PipelineIDs for testing connectors. | ||
extension.ComponentStatusChanged now takes in a *pipeline.InstanceID instead of a *component.InstanceID. | ||
extensiontest.NewStatusWatcherExtensionFactory now takes in a *pipeline.InstanceID instead of a *component.InstanceID. | ||
extensions.NotifyComponentStatusChange now takes in a *pipeline.InstanceID instead of a *component.InstanceID. | ||
|
||
# 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'] |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,12 +9,13 @@ import ( | |
"strings" | ||
) | ||
|
||
// typeAndNameSeparator is the separator that is used between type and name in type/name composite keys. | ||
const typeAndNameSeparator = "/" | ||
// TypeAndNameSeparator is the separator that is used between type and name in type/name composite keys. | ||
const TypeAndNameSeparator = "/" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should be renamed since we also use it for separating |
||
|
||
// 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:"-"` | ||
|
@@ -62,7 +63,7 @@ func (id ID) MarshalText() (text []byte, err error) { | |
// UnmarshalText implements the encoding.TextUnmarshaler interface. | ||
func (id *ID) UnmarshalText(text []byte) error { | ||
idStr := string(text) | ||
items := strings.SplitN(idStr, typeAndNameSeparator, 2) | ||
items := strings.SplitN(idStr, TypeAndNameSeparator, 2) | ||
var typeStr, nameStr string | ||
if len(items) >= 1 { | ||
typeStr = strings.TrimSpace(items[0]) | ||
|
@@ -73,14 +74,14 @@ func (id *ID) UnmarshalText(text []byte) error { | |
} | ||
|
||
if typeStr == "" { | ||
return fmt.Errorf("in %q id: the part before %s should not be empty", idStr, typeAndNameSeparator) | ||
return fmt.Errorf("in %q id: the part before %s should not be empty", idStr, TypeAndNameSeparator) | ||
} | ||
|
||
if len(items) > 1 { | ||
// "name" part is present. | ||
nameStr = strings.TrimSpace(items[1]) | ||
if nameStr == "" { | ||
return fmt.Errorf("in %q id: the part after %s should not be empty", idStr, typeAndNameSeparator) | ||
return fmt.Errorf("in %q id: the part after %s should not be empty", idStr, TypeAndNameSeparator) | ||
} | ||
} | ||
|
||
|
@@ -99,5 +100,5 @@ func (id ID) String() string { | |
return id.typeVal.String() | ||
} | ||
|
||
return id.typeVal.String() + typeAndNameSeparator + id.nameVal | ||
return id.typeVal.String() + TypeAndNameSeparator + id.nameVal | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a
here?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
:There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andcomponent.Kind
to internal and make shims also for these two oncomponent
. It sounds like a pain, so let's first discuss the rest of the PRThere was a problem hiding this comment.
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.