[schema processor] Part 3 - Modifiers and Revisions #12147

MovieStoreGuy · 2022-07-07T07:01:21Z

Description:
An extension of #12146

This PR adds in support for Modifier an incoming signal type and a Revision which applies the modifiers

tigrannajaryan · 2022-07-26T19:27:57Z

Please rebase.

As the schema processor improves, this package path changes to help make sense of what is trying to do.

A modifier allows mutating a signal from a previous version to the current one and vice versa.

A revision handles applying the modifiers to incoming signals and converting a signal to the next version.

MovieStoreGuy · 2022-07-26T22:54:32Z

@tigrannajaryan would it also make sense to remove the change log for this PR as well?

Edit: There was no external facing change here that impact users, removing the changelog.

tigrannajaryan · 2022-07-27T18:50:24Z

processor/schemaprocessor/internal/translation/modifier.go

+			// Interesting thing of note,
+			// ordering of these methods is important
+			// otherwise you will get repeated values


The comment is a bit imprecise. What really matters that v returned by Get is a reference. If you call Remove it results in v reference to become invalid (it will likely point to some other attribute, but it can also point past the slice, so be totally undefined). So, this requires that v must be used before Remove is called.
Perhaps remove the "otherwise you will get repeated values" part, since it can be worse than that.

tigrannajaryan · 2022-07-27T18:53:31Z

processor/schemaprocessor/internal/translation/modifier.go

+
+// modify is one change that can be made to a signal
+type modify struct {
+	names map[string]string


It may be useful to use type aliases from "go.opentelemetry.io/otel/schema" instead of string, so that it is clear what string this is, (e.g. AttributeName).

tigrannajaryan · 2022-07-27T18:54:00Z

processor/schemaprocessor/internal/translation/modifier.go

+	names map[string]string
+
+	appliesTo map[string]struct{}
+	attrs     map[string]string


It may be useful to use type aliases from "go.opentelemetry.io/otel/schema" instead of string, so that it is clear what string this is, (e.g. AttributeName).

Same for other strings in this struct.

I believe the reason why I didn't do that is that go defines them as new types instead of aliases so some operations had extra cruft that I cut out by keeping it as just strings.

tigrannajaryan · 2022-07-27T18:56:07Z

processor/schemaprocessor/internal/translation/modifier.go

+type modify struct {
+	names map[string]string
+
+	appliesTo map[string]struct{}


Add comment to explain that if this is empty then the modification applies to everything (as opposed to nothing).

tigrannajaryan · 2022-07-27T18:58:59Z

processor/schemaprocessor/internal/translation/modifier_race_test.go

+	s.Attributes().Sort()
+	s.SetName("foo")
+
+	fixture.ParallelRaceCompute(t, 100, func() error {


What does this test verify? Since we are using Clone then the test can't fail. Are we verifying that Clone works correctly?

At this point I was validating that concurrent access to the modifiers would not cause race condition.

This is more a safety check more than anything else.

tigrannajaryan · 2022-07-27T19:15:17Z

processor/schemaprocessor/internal/translation/modifier.go

+
+// Modifier abstracts a verion change that allows
+// for the changes to be updated or revert.
+type Modifier interface {


This interface is probably fine for now, but it likely need to change because we have recently introduced more complicated changes. The Schema File Format 1.1 brought the "split" transformation which changes both the metric name and the attribute name at the same time: https://github.com/open-telemetry/opentelemetry-go/blob/1eae91b3b099af62100cd630cd1cab89fe841615/schema/v1.1/ast/metrics.go#L40

Yeah, so thinking about this I believe the Modifier may also need to be versioned in code to ensure behaviour.

MovieStoreGuy · 2022-08-09T08:11:15Z

Sorry, I do intend to get back to this however, I am busy with some internal work atm.

github-actions · 2022-09-16T05:40:42Z

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

github-actions · 2022-09-30T05:41:20Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

MovieStoreGuy

Just trying to revive an old PR, sorry for the absence. A lot has happened for me these months.

MovieStoreGuy · 2022-11-10T11:54:40Z

processor/schemaprocessor/internal/translation/modifier_race_test.go

+	s.Attributes().Sort()
+	s.SetName("foo")
+
+	fixture.ParallelRaceCompute(t, 100, func() error {


At this point I was validating that concurrent access to the modifiers would not cause race condition.

This is more a safety check more than anything else.

MovieStoreGuy · 2022-11-10T11:56:12Z

processor/schemaprocessor/internal/translation/modifier.go

+	names map[string]string
+
+	appliesTo map[string]struct{}
+	attrs     map[string]string


I believe the reason why I didn't do that is that go defines them as new types instead of aliases so some operations had extra cruft that I cut out by keeping it as just strings.

MovieStoreGuy · 2022-11-10T11:57:13Z

processor/schemaprocessor/internal/translation/modifier.go

+
+// Modifier abstracts a verion change that allows
+// for the changes to be updated or revert.
+type Modifier interface {


Yeah, so thinking about this I believe the Modifier may also need to be versioned in code to ensure behaviour.

tigrannajaryan · 2022-11-10T20:03:38Z

Just trying to revive an old PR, sorry for the absence. A lot has happened for me these months.

@MovieStoreGuy Welcome back! Looks like I cannot reopen the PR. You may need to create a new PR and link to this one.

* [schema processor] Modifier A modifier allows mutating a signal from a previous version to the current one and vice versa. [schema processor] Adding revisions A revision handles applying the modifiers to incoming signals and converting a signal to the next version. * Adding changes * Adding migrate package. Following patterns similar to databases where you'd apply changes and rollback. the Migrate packages follows these designs to help make it clear what it is trying to do. * Removing modiferies to avoid confusion * Fixing up naming issues * Adding checks in for building a revision * Fixing up replaces * Removing transform files * Fixing naming * Fixing up conflict * Fixing typo * Updating naming * Fixing up ast usage * FIxing up generated mod files * Fixing up license headers

tigrannajaryan · 2023-06-28T23:55:06Z

@MovieStoreGuy do you plan to reopen Part 4?

MovieStoreGuy requested review from a team and TylerHelmuth July 7, 2022 07:01

github-actions bot assigned bogdandrutu Jul 7, 2022

This was referenced Jul 7, 2022

[schema processor] Part 4 - Translations and management #12148

Closed

[schema processor] A complete implementation #11547

Closed

MovieStoreGuy changed the title ~~[schema processor] part 3 - Modifiers and Revisions~~ [schema processor] Part 3 - Modifiers and Revisions Jul 7, 2022

MovieStoreGuy force-pushed the msg/part-3-schema-revisions branch from 0b1ccac to 7c3825a Compare July 17, 2022 05:44

MovieStoreGuy added 7 commits July 27, 2022 08:07

[Schema Processor] Updating internal package from schema to translation

6c58727

As the schema processor improves, this package path changes to help make sense of what is trying to do.

[Schema Processor] Updating internal package from schema to translation

864f092

As the schema processor improves, this package path changes to help make sense of what is trying to do.

[schema processor] Modifier

f4e1506

A modifier allows mutating a signal from a previous version to the current one and vice versa.

[schema processor] Adding revisions

8e5f246

A revision handles applying the modifiers to incoming signals and converting a signal to the next version.

Adding changelog

230ce5a

adding goporto statements

89f83fa

Fixing mod

0674cf7

MovieStoreGuy force-pushed the msg/part-3-schema-revisions branch from 2d0f70b to 0674cf7 Compare July 26, 2022 22:46

Removing changelog for internal change

ad5df1b

tigrannajaryan reviewed Jul 27, 2022

View reviewed changes

tigrannajaryan added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 27, 2022

github-actions bot added the Stale label Sep 16, 2022

github-actions bot closed this Sep 30, 2022

MovieStoreGuy commented Nov 10, 2022

View reviewed changes

MovieStoreGuy mentioned this pull request Dec 14, 2022

[schema processor] Part 3 - Modifiers and Revisions #12147 #17020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[schema processor] Part 3 - Modifiers and Revisions #12147

[schema processor] Part 3 - Modifiers and Revisions #12147

MovieStoreGuy commented Jul 7, 2022

tigrannajaryan commented Jul 26, 2022

MovieStoreGuy commented Jul 26, 2022 •

edited

Loading

tigrannajaryan Jul 27, 2022

tigrannajaryan Jul 27, 2022

tigrannajaryan Jul 27, 2022

MovieStoreGuy Nov 10, 2022

tigrannajaryan Jul 27, 2022

tigrannajaryan Jul 27, 2022

MovieStoreGuy Nov 10, 2022

tigrannajaryan Jul 27, 2022

MovieStoreGuy Nov 10, 2022

MovieStoreGuy commented Aug 9, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 30, 2022

MovieStoreGuy left a comment

MovieStoreGuy Nov 10, 2022

MovieStoreGuy Nov 10, 2022

MovieStoreGuy Nov 10, 2022

tigrannajaryan commented Nov 10, 2022

tigrannajaryan commented Jun 28, 2023

[schema processor] Part 3 - Modifiers and Revisions #12147

[schema processor] Part 3 - Modifiers and Revisions #12147

Conversation

MovieStoreGuy commented Jul 7, 2022

tigrannajaryan commented Jul 26, 2022

MovieStoreGuy commented Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy commented Aug 9, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 30, 2022

MovieStoreGuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Nov 10, 2022

tigrannajaryan commented Jun 28, 2023

MovieStoreGuy commented Jul 26, 2022 •

edited

Loading