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

Schema Processor Revamp [Part 1] - Operators and Migrators #35214

Conversation

ankitpatel96
Copy link
Contributor

@ankitpatel96 ankitpatel96 commented Sep 16, 2024

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

@ankitpatel96 ankitpatel96 requested a review from a team September 16, 2024 17:46
@github-actions github-actions bot added the processor/schema Schema processor label Sep 16, 2024
@ankitpatel96 ankitpatel96 force-pushed the ankit-schema-processor-operator branch 2 times, most recently from 453e629 to 4027afa Compare September 17, 2024 22:44
@ankitpatel96 ankitpatel96 force-pushed the ankit-schema-processor-operator branch from 4027afa to 39b4026 Compare September 20, 2024 20:53
@ankitpatel96 ankitpatel96 requested a review from a team as a code owner September 20, 2024 20:53
}

// LogAttributeOperator is an Operator that acts on LogRecords' attributes
type LogAttributeOperator struct {
Copy link
Contributor

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?

Copy link
Contributor Author

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_name.go Outdated Show resolved Hide resolved
processor/schemaprocessor/internal/operator/signal_name.go Outdated Show resolved Hide resolved
@ankitpatel96 ankitpatel96 force-pushed the ankit-schema-processor-operator branch from 39b4026 to 20ec9bc Compare September 30, 2024 20:18
@ankitpatel96
Copy link
Contributor Author

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

@ankitpatel96 ankitpatel96 force-pushed the ankit-schema-processor-operator branch from 875f10b to 4bd85ee Compare October 15, 2024 19:20
Copy link
Contributor Author

@ankitpatel96 ankitpatel96 left a 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_name.go Outdated Show resolved Hide resolved
processor/schemaprocessor/internal/operator/signal_name.go Outdated Show resolved Hide resolved
return nil
}

func (o SpanAttributeOperator) Rollback(data any) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

processor/schemaprocessor/internal/migrate/conditional.go Outdated Show resolved Hide resolved
}

// LogAttributeOperator is an Operator that acts on LogRecords' attributes
type LogAttributeOperator struct {
Copy link
Contributor Author

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/migrate/conditional.go Outdated Show resolved Hide resolved
@ankitpatel96
Copy link
Contributor Author

I've addressed all your comments! Thanks for the thoughtful suggestions

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 31, 2024
@@ -0,0 +1,28 @@
# Design
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@ankitpatel96 ankitpatel96 Nov 7, 2024

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!

@MovieStoreGuy MovieStoreGuy added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 6, 2024
@ankitpatel96 ankitpatel96 force-pushed the ankit-schema-processor-operator branch from 82ac0f4 to f51e1cf Compare November 7, 2024 21:53
@ankitpatel96 ankitpatel96 force-pushed the ankit-schema-processor-operator branch from f51e1cf to 8b3c62b Compare November 7, 2024 22:02
@ankitpatel96
Copy link
Contributor Author

ankitpatel96 commented Nov 7, 2024

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

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 22, 2024
@songy23 songy23 added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Nov 22, 2024
mx-psi added a commit that referenced this pull request Dec 3, 2024
…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]>
shivanthzen pushed a commit to shivanthzen/opentelemetry-collector-contrib that referenced this pull request Dec 5, 2024
…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]>
ZenoCC-Peng pushed a commit to ZenoCC-Peng/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2024
…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]>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
never stale Issues marked with this label will be never staled and automatically removed processor/schema Schema processor Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants