-
Notifications
You must be signed in to change notification settings - Fork 2.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
[chore] Schema Processor Revamp Implementation Parent PR #35248
base: main
Are you sure you want to change the base?
[chore] Schema Processor Revamp Implementation Parent PR #35248
Conversation
@ankitpatel96 thank you for working on this. Are you looking for reviews on this PR or we should wait until you split into smaller ones? |
Hi @tigrannajaryan, I am definitely not trying to get a review on this entire PR haha - I am in the process of splitting it into 3-4 PRs. Part 1 - #35214 The rest of the code is largely done - I'm just working on splitting it into stacked PRs that makes sense. I am working up a mini design document so people have an idea of what they are looking at. |
Edit: Added link to specification One major question -> Is this not intended to support I was going through looking for the intricate "fun" code to deal with that scenario, but I didn't see anything, nor do I see tests associated with it. I think it's fine for to disallow that for an initial implementation/improvement, but you may want to consider the architecture/design for when you need to apply attirbute name changes on resources in addition to signals. Specifically -> The shuffling of Data points out of a resource and into a new one is... fun. From my SDK implementation this was much simpler, as the SDK only allows one resource. For the Collector -> I think you'll need some robust code, checks and performance tests around that. See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/schemas/file_format_v1.1.0.md#all-section - specifically the notion there's a resource attribute rename possibility and that |
Hi Josh, |
9c0ad9c
to
1829df1
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
1829df1
to
472cedb
Compare
472cedb
to
af3a4fd
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
af3a4fd
to
12aeb95
Compare
12aeb95
to
58256c1
Compare
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]>
58256c1
to
64aedc8
Compare
Hi, this is ready for review! @tigrannajaryan @MovieStoreGuy please take a look! Much of this is @MovieStoreGuy's original work - it works to lookup and cache the schema files as well as orchestrating the rest of the code / hooking it up to the collector factory. The vast majority of this PR is tests that I've written that are pretty end-to-end. |
…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]>
@ankitpatel96 do you plan to split this into smaller bits? It is over 4K line changes, hard to review such a big PR. |
I can definitely try - I will say that many many (more than half) of these changes are test schema files, and input and output OTLP payloads. I will try to split up this PR further before next week if that works for you |
Please do if there is a reasonable way to do it. If you can't, that's OK, but it will likely delay my review, I will need to find a bigger chunk of time to review this PR. |
@ankitpatel96 please ping me when you have a PR to review. |
fa1989e
to
2e80a66
Compare
@tigrannajaryan I've pushed up 2 more parts - |
2e80a66
to
ecf3957
Compare
I will block out some time to review this detail. |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This is a slice of changes from #35248 This PR details Translation which defines the complete abstraction of schema translation file. It manages Revisions to move between two versions of a schema <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests
Description:
This is a duplicate of #35213 - I had to reopen this because the Github Actions refused to trigger.
This is the schema processor, as originally written by @MovieStoreGuy in #11547 - refreshed and rebased. I've rewritten some parts and added significant amounts of testing to many parts - resulting in quite a few changes. I will be splitting this PR into many parts.
There are several features that need to be completed before this is ready for production use:
This only supports the Schema File Format 1.0.0 - we need to support 1.1.0
Right now only downgrades work. Upgrades need some work to enable
We should improve internal metrics and observability.