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

[processor/schematransformerprocessor] Initial PR #8371

Conversation

MovieStoreGuy
Copy link
Contributor

Description:
As part of the accepted specification, there needs to be a processor within the collector that is able to allow for transformations of semantic convention versions . This is my initial draft, I wrote this before https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/8344/files shared their solution to it.

Link to tracking Issue:
#8495
Original raised in the collector but should be shifted to the contrib a I can tell.

Testing:
It currently does nothing and there is the initial testing set up within the components.

Documentation:
Added a minimal README, will expand upon it once more work has been added to the collector.

@MovieStoreGuy MovieStoreGuy marked this pull request as ready for review March 11, 2022 01:30
@MovieStoreGuy MovieStoreGuy requested review from a team and jpkrohling March 11, 2022 01:30
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch 2 times, most recently from 6ac440e to 5e039b9 Compare March 15, 2022 02:58
@MovieStoreGuy
Copy link
Contributor Author

I have avoided adding any implementation detail here to keep the high level PR in, this PR can should be fine to merge in and run (since it will only just validate the configuration, passthrough exactly what it receives).

Not entirely sure how to resolve the issue within the gomoddownload step, @codeboten if you have any ideas please let me know :)

After this initial PR goes in, the next logical flow I have in mind is:

  • Figure out the best way to cache schemas
  • Determine schema versions and applying changes with minimal efforts
  • Adopt @tigrannajaryan compiled schemas to reduce runtime efforts
  • Add in directory extension support to write schemas to disk

go.mod Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch from b1483ad to e5136ea Compare March 16, 2022 01:13
@Aneurysm9
Copy link
Member

You'll also need to add a replace directive to cmd/configschema/go.mod as it depends on the root module and replace directives are only honored from the current go.mod for any operation. It's super annoying and we're working on a tool to make this a non-issue, but until then some hand-crafting is necessary.

@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch from a7abf33 to b532668 Compare March 16, 2022 02:44
@MovieStoreGuy
Copy link
Contributor Author

Thanks @Aneurysm9, it looks like it managed to get past the gomoddownload part and getting onto the meaningful tests :)

@MovieStoreGuy
Copy link
Contributor Author

Nobody move... the load tests somehow passed.

@codeboten , @jpkrohling, @Aneurysm9 , if you have time to have a look over this pull request?

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Sorry about that, but we just had a new release, so, the go.mod/sum files here have to be changed.

CHANGELOG.md Outdated Show resolved Hide resolved
processor/schematransformerprocessor/config.go Outdated Show resolved Hide resolved
// See the License for the specific language governing permissions and
// limitations under the License.

package schematransformerprocessor_test
Copy link
Member

Choose a reason for hiding this comment

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

Are tests coming to this file in the next 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.

Yes, since the transformer is currently just an empty struct, I wasn't exactly sure how the transformer would look like so I didn't want to burden reviewers with more work than required.

processor/schematransformerprocessor/testdata/schema.yml Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch 2 times, most recently from f6d815f to 73551e6 Compare March 17, 2022 01:03
@MovieStoreGuy
Copy link
Contributor Author

Lame, the loadtest flaked out on me.

@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch 2 times, most recently from 81c7ae5 to c1006e6 Compare March 18, 2022 03:11
@MovieStoreGuy
Copy link
Contributor Author

The stairs have aligned once again! Once I get a chance, I will have a stab to see if I can help reduce the flake in the those tests.

processor/schematransformerprocessor/config.go Outdated Show resolved Hide resolved
processor/schematransformerprocessor/config.go Outdated Show resolved Hide resolved
processor/schematransformerprocessor/config.go Outdated Show resolved Hide resolved
processor/schematransformerprocessor/README.md Outdated Show resolved Hide resolved
processor/schematransformerprocessor/README.md Outdated Show resolved Hide resolved
processor/schematransformerprocessor/testdata/config.yml Outdated Show resolved Hide resolved
processor/schematransformerprocessor/testdata/schema.yml Outdated Show resolved Hide resolved
processor/schematransformerprocessor/README.md Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch from c1006e6 to 8e293cd Compare March 21, 2022 01:31
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch 2 times, most recently from b19fafd to e2c8a9b Compare March 30, 2022 06:46
@MovieStoreGuy
Copy link
Contributor Author

Sorry @tigrannajaryan,

Finally got a chance to get back to this :)
If you have a moment to look over this it would be greatly appreciated 🙏🏽

@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch 3 times, most recently from 5b48af9 to 8c4d72f Compare March 30, 2022 08:01
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch 2 times, most recently from 401ff1b to d364da8 Compare April 22, 2022 03:11
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Just a couple of suggestions and questions. Please add a note about what constitutes a family in the readme file, it can be a copy paste of the definition in the schema overview, but i think it will help users of this processor

CHANGELOG.md Outdated Show resolved Hide resolved

## Caching Schema Translation Files

In order to improve effeicency of the processor, the `precache` option allows the processor to start downloading and preparing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In order to improve effeicency of the processor, the `precache` option allows the processor to start downloading and preparing
In order to improve efficency of the processor, the `precache` option allows the processor to start downloading and preparing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if that was a mistake, but misspell is reporting it as a typo.

processor/schemaprocessor/README.md Outdated Show resolved Hide resolved
processor/schemaprocessor/factory.go Outdated Show resolved Hide resolved
processor/schemaprocessor/testdata/config.yml Outdated Show resolved Hide resolved
jpkrohling
jpkrohling previously approved these changes Apr 22, 2022
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. Just clarify to users what happens when conflicting versions of the schemas are used, and fix a couple of typos.

processor/schemaprocessor/README.md Outdated Show resolved Hide resolved
processor/schemaprocessor/README.md Outdated Show resolved Hide resolved
processor/schemaprocessor/config.go Show resolved Hide resolved
@jpkrohling jpkrohling dismissed their stale review April 22, 2022 19:58

Retracting, due to the changelog changes.

@jpkrohling
Copy link
Member

Sorry, I'm retracting my approval because of the unwanted changes to the changelog.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Apart from the couple of typos, LGTM!

processor/schemaprocessor/config.go Outdated Show resolved Hide resolved
processor/schemaprocessor/transformer_test.go Outdated Show resolved Hide resolved
processor/schemaprocessor/transformer_test.go Outdated Show resolved Hide resolved
processor/schemaprocessor/transformer_test.go Outdated Show resolved Hide resolved
processor/schemaprocessor/transformer_test.go Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch from 92b91ad to 6610e95 Compare April 26, 2022 22:59
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch from 6610e95 to 2be9365 Compare April 28, 2022 23:34
@MovieStoreGuy
Copy link
Contributor Author

@jpkrohling 🙏🏽

Could I get this merged please?

@jpkrohling
Copy link
Member

I tried to update this branch, but got an error :-( The changelog needs updating, and there's a conflict with the go.sum versions. Once this is fixed and the build is green, I'll merge (if this happens today).

@tigrannajaryan
Copy link
Member

@MovieStoreGuy ping me when the conflicts are resolved, I will merge.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments, please rebase and we'll get this merge right away

@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch from 2be9365 to 64ccae4 Compare May 1, 2022 23:53
This is the initial commit into the collector to add in the component to
help enforce a standard semantic convention for all signals.
@MovieStoreGuy MovieStoreGuy force-pushed the msg/schema-transformer-processor branch from 64ccae4 to 5f83e82 Compare May 1, 2022 23:54
@tigrannajaryan tigrannajaryan merged commit 35db7b8 into open-telemetry:main May 2, 2022
@tigrannajaryan
Copy link
Member

Thank you @MovieStoreGuy
Looking forward to the continuation PRs!

djaglowski pushed a commit to djaglowski/opentelemetry-collector-contrib that referenced this pull request May 10, 2022
**Description:**
As part of the [accepted specification](https://github.com/open-telemetry/opentelemetry-specification/blob/e16e1a781bdf62a8fcfa1a12963844749de23e7a/specification/schemas/overview.md#collector-assisted-schema-transformation), there needs to be a processor within the collector that is able to allow for transformations of semantic convention versions . This is my initial draft, I wrote this before https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/8344/files shared their solution to it.

**Link to tracking Issue:**
open-telemetry#8495
Original raised in the collector but should be shifted to the contrib a I can tell.

**Testing:**
It currently does nothing and there is the initial testing set up within the components.

**Documentation:** 
Added a minimal README, will expand upon it once more work has been added to the collector.
@tigrannajaryan
Copy link
Member

@MovieStoreGuy do you plan to continue working on the schema processor?

@MovieStoreGuy
Copy link
Contributor Author

Yes, sorry. I have been meaning to get back into it but I have been working on rollout infra changes to adopt the otel collector for resource metrics, so that has taken a lot of brain space atm.

I intend to get back to this this week

@tigrannajaryan
Copy link
Member

Sounds good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants