-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Schema Processor Revamp [Part 1] - Operators and Migrators #35214
Schema Processor Revamp [Part 1] - Operators and Migrators #35214
Conversation
453e629
to
4027afa
Compare
4027afa
to
39b4026
Compare
} | ||
|
||
// LogAttributeOperator is an Operator that acts on LogRecords' attributes | ||
type LogAttributeOperator struct { |
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.
Could I ask that new types get moved into their own file to keep files concise?
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.
At this point there are some 11 structs in operator
, so that would be a lot of files. I'd prefer to not do that - but am open to changing my mind if you feel very strongly.
processor/schemaprocessor/internal/operator/signal_interfaces.go
Outdated
Show resolved
Hide resolved
processor/schemaprocessor/internal/operator/attributes_operators.go
Outdated
Show resolved
Hide resolved
processor/schemaprocessor/internal/operator/attributes_operators.go
Outdated
Show resolved
Hide resolved
processor/schemaprocessor/internal/operator/attributes_operators.go
Outdated
Show resolved
Hide resolved
39b4026
to
20ec9bc
Compare
Sorry for the delay - I'm still actively working on this. I was out sick for a while and had some preplanned personal events. Now that I'm back - I'll be iterating on this PR |
875f10b
to
4bd85ee
Compare
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.
Sorry for the long delay all - but I'm back and looking forward to working through these PRs with ya'll!
I have addressed most of your comments - the major item remaining is renaming Operator
. I have also gone ahead and added a Design document to this PR to help everyone understand the overall sketch of this code.
processor/schemaprocessor/internal/operator/signal_interfaces.go
Outdated
Show resolved
Hide resolved
return nil | ||
} | ||
|
||
func (o SpanAttributeOperator) Rollback(data any) error { |
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.
fixed
processor/schemaprocessor/internal/operator/attributes_operators.go
Outdated
Show resolved
Hide resolved
processor/schemaprocessor/internal/operator/attributes_operators.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
// LogAttributeOperator is an Operator that acts on LogRecords' attributes | ||
type LogAttributeOperator struct { |
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.
At this point there are some 11 structs in operator
, so that would be a lot of files. I'd prefer to not do that - but am open to changing my mind if you feel very strongly.
I've addressed all your comments! Thanks for the thoughtful suggestions |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@@ -0,0 +1,28 @@ | |||
# Design |
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.
Thank you for adding this document, it is very useful.
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.
It is really awesome, and I also had no idea github support mermaid diagrams.
100% going to start doing this more often.
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.
Yay I'm glad ya'll like it! I contributed one to the collector too at https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/internal-architecture.md. I hope to make more!
82ac0f4
to
f51e1cf
Compare
f51e1cf
to
8b3c62b
Compare
Thank you so much for your reviews @tigrannajaryan and @MovieStoreGuy - I realize I forgot to mention that this PR is not mergeable - it conflicts with a bunch of existing code. #35267 is a stacked PR / superset of these changes and should be safe to merge (when it's done) - so would ask y'all to start looking at it when you get the chance. It's actually quite short! Changed files from the next PR are processor/schemaprocessor/internal/changelist/changelist.go I'm asking a maintainer if they would be willing to push a copy of this branch to the core repo so I can switch the base of the other PR to this - thus only the stacked changes would be shown. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…35267) **Description:** <Describe what has changed.> This is a slice of changes from #35248 This PR details how operators are used to build the execution pipeline for a given schemafile. Changed files from the [previous PR](#35214) are: processor/schemaprocessor/internal/changelist/changelist.go processor/schemaprocessor/internal/translation/revision_v1.go processor/schemaprocessor/internal/translation/revision_v1_test.go processor/schemaprocessor/go.mod I'm asking a maintainer if they would be willing to push a copy of the previous PR's branch to the core repo so I can switch the base of this PR to the previous PR - thus only the stacked changes would be shown. Edit: this is apparently not easily supported - so asking reviewers to just focus on the changed files listed above. Sorry about that! **Testing:** <Describe what testing was performed and which tests were added.> Unit tests --------- Co-authored-by: Pablo Baeyens <[email protected]>
…pen-telemetry#35267) **Description:** <Describe what has changed.> This is a slice of changes from open-telemetry#35248 This PR details how operators are used to build the execution pipeline for a given schemafile. Changed files from the [previous PR](open-telemetry#35214) are: processor/schemaprocessor/internal/changelist/changelist.go processor/schemaprocessor/internal/translation/revision_v1.go processor/schemaprocessor/internal/translation/revision_v1_test.go processor/schemaprocessor/go.mod I'm asking a maintainer if they would be willing to push a copy of the previous PR's branch to the core repo so I can switch the base of this PR to the previous PR - thus only the stacked changes would be shown. Edit: this is apparently not easily supported - so asking reviewers to just focus on the changed files listed above. Sorry about that! **Testing:** <Describe what testing was performed and which tests were added.> Unit tests --------- Co-authored-by: Pablo Baeyens <[email protected]>
…pen-telemetry#35267) **Description:** <Describe what has changed.> This is a slice of changes from open-telemetry#35248 This PR details how operators are used to build the execution pipeline for a given schemafile. Changed files from the [previous PR](open-telemetry#35214) are: processor/schemaprocessor/internal/changelist/changelist.go processor/schemaprocessor/internal/translation/revision_v1.go processor/schemaprocessor/internal/translation/revision_v1_test.go processor/schemaprocessor/go.mod I'm asking a maintainer if they would be willing to push a copy of the previous PR's branch to the core repo so I can switch the base of this PR to the previous PR - thus only the stacked changes would be shown. Edit: this is apparently not easily supported - so asking reviewers to just focus on the changed files listed above. Sorry about that! **Testing:** <Describe what testing was performed and which tests were added.> Unit tests --------- Co-authored-by: Pablo Baeyens <[email protected]>
…pen-telemetry#35267) **Description:** <Describe what has changed.> This is a slice of changes from open-telemetry#35248 This PR details how operators are used to build the execution pipeline for a given schemafile. Changed files from the [previous PR](open-telemetry#35214) are: processor/schemaprocessor/internal/changelist/changelist.go processor/schemaprocessor/internal/translation/revision_v1.go processor/schemaprocessor/internal/translation/revision_v1_test.go processor/schemaprocessor/go.mod I'm asking a maintainer if they would be willing to push a copy of the previous PR's branch to the core repo so I can switch the base of this PR to the previous PR - thus only the stacked changes would be shown. Edit: this is apparently not easily supported - so asking reviewers to just focus on the changed files listed above. Sorry about that! **Testing:** <Describe what testing was performed and which tests were added.> Unit tests --------- Co-authored-by: Pablo Baeyens <[email protected]>
This is a slice of changes from #35248
This PR is the first one to be reviewed - its broader context can be seen in the parent PR.
A Migrator does the actual migration of a piece of data - things like renaming attributes or changing the name of a signal. An Operator operates on a typed signal - things like a SpanEvent or a LogRecord. The Operator knows how to call a Migrator for its given DataType.
The next PR will detail how operators are used to build the execution pipeline for a given schemafile.
Edit: This PR is not mergeable - it conflicts with a bunch of existing code. #35267 is a stacked PR / superset of these changes and should be safe to merge