Add schema transformation functionality to resource creation and merge #4503

MrAlias · 2023-09-12T21:11:40Z

Currently, the resource package fails for conflicting schema URL merges in the following way:

If New is used with multiple detectors that detect resources at with different schema URLs
If Detect is used with multiple detectors that detect resources at with different schema URLs
If Merge is provided two resources with different schema URLs

All of these errors can be reduced to the last one: the New function will call the underlying detect which itself calls Merge for multiple resources.

This fixes all of these failures by upgrading the older resources prior to merging using schema transformations.

There are no API changes added at this time. The translation is limited to only upgrading and only in the Merge function. It is possible to add more customizable translations that support more schema (other than just OpenTelemetry ones), but that is left for another PR once there is deemed enough user support.

Fixes #2341
Fixes #3769
Fixes #4476
Fixes open-telemetry/opentelemetry-go-contrib#3657

Design

Based on https://github.com/MrAlias/otel-schema-utils/tree/main with the following notable differences:

Update the Merge function in sdk/resource to upgrade the less of the two passed resources
- Detect calls detect which uses Merge to resolve resources from multiple detectors
- New uses detect to create new resources with Detectors
- Only upgrades are currently supported
Add the sdk/resource/internal/schema package
- The Version function parses and converts schema URL to semconv.Version types.
- The Upgrade function allows the upgrade of a resources attributes using a statically defined transform from an OTel schema
  - The transforms are statically defined using a generation script in generation
    - The latest OTel schema is held in code in the schema directory
      - The a fetch.go script is included to fetch a schema and can be used for upgrade
  - A render package is included that will render and format Go to generate the template.go file with the static transforms
- No panics during start ups, which would be the case for dynamically loading these schema
- No HTTP calls during runtime, the HTTP fetch operations are performed by our CI system or locally

codecov · 2023-09-13T19:56:55Z

Codecov Report

Merging #4503 (74e8197) into main (a5190f6) will decrease coverage by 0.1%.
The diff coverage is 79.0%.

@@           Coverage Diff           @@
##            main   #4503     +/-   ##
=======================================
- Coverage   81.3%   81.3%   -0.1%     
=======================================
  Files        220     222      +2     
  Lines      17678   17825    +147     
=======================================
+ Hits       14386   14499    +113     
- Misses      2992    3020     +28     
- Partials     300     306      +6

Files Changed	Coverage
...resource/internal/schema/generate/render/render.go	`42.5%`
sdk/resource/internal/schema/schema.go	`100.0%`
sdk/resource/resource.go	`100.0%`

There is only one schema needed now. Keep it in code.

Add suggested tests from review.

pellared · 2023-09-19T11:47:59Z

sdk/go.mod

 	github.com/go-logr/logr v1.2.4
 	github.com/google/go-cmp v0.5.9
 	github.com/stretchr/testify v1.8.4
 	go.opentelemetry.io/otel v1.19.0-rc.1
+	go.opentelemetry.io/otel/schema v0.0.6


We are introducing on a new unstable Go module.
If I am not mistaken I have found that this module is currently used by:

https://github.com/open-telemetry/build-tools/tree/main/schemas

https://github.com/MrAlias/otel-schema-utils

Still somebody else can potentially use go.opentelemetry.io/otel/schema and then we can have a problem in case there would be some incompatible changes in go.opentelemetry.io/otel/schema.

Currently, I have two ideas how we could resolve the issue:

A) Stabilize go.opentelemetry.io/otel/schema.
B) Use gotmpl to copy (vendor) ./schema content.

Are there any reasons why not going with option A? Any other ideas?

Tracking stability here: https://github.com/orgs/open-telemetry/projects/64/views/1

Private project @MrAlias ?

IIRC you need to be an OTel member to see the project.

sdk/go.mod

pellared · 2023-09-19T11:53:18Z

sdk/resource/internal/schema/generate/render/render_test.go

+package cmd
+
+import (
+	"bytes"
+	"io"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	sUtil "go.opentelemetry.io/otel/schema/v1.1"
+)
+
+func TestRender(t *testing.T) {
+	schemaF, err := os.Open("./testdata/schema.yaml")
+	require.NoError(t, err)
+	defer schemaF.Close()
+
+	s, err := sUtil.Parse(schemaF)
+	require.NoError(t, err)
+
+	wantF, err := os.Open("./testdata/transforms.go")
+	require.NoError(t, err)
+
+	var want bytes.Buffer
+	_, err = io.Copy(&want, wantF)
+	require.NoError(t, err)
+
+	e, err := entries(s)
+	require.NoError(t, err)
+
+	var got bytes.Buffer
+	require.NoError(t, render(&got, e))
+
+	assert.Equal(t, want.String(), got.String())
+}


schema "go.opentelemetry.io/otel/schema/v1.1" instead of sUtil "go.opentelemetry.io/otel/schema/v1.1" (it is even suggested in https://pkg.go.dev/go.opentelemetry.io/otel/schema

Use os.ReadFile

Move want preparation on the top.

Suggested change

package cmd

import (

"bytes"

"io"

"os"

"testing"

"github.com/stretchr/testify/assert"

"github.com/stretchr/testify/require"

sUtil "go.opentelemetry.io/otel/schema/v1.1"

)

func TestRender(t *testing.T) {

schemaF, err := os.Open("./testdata/schema.yaml")

require.NoError(t, err)

defer schemaF.Close()

s, err := sUtil.Parse(schemaF)

require.NoError(t, err)

wantF, err := os.Open("./testdata/transforms.go")

require.NoError(t, err)

var want bytes.Buffer

_, err = io.Copy(&want, wantF)

require.NoError(t, err)

e, err := entries(s)

require.NoError(t, err)

var got bytes.Buffer

require.NoError(t, render(&got, e))

assert.Equal(t, want.String(), got.String())

}

package cmd

import (

"bytes"

"os"

"testing"

"github.com/stretchr/testify/assert"

"github.com/stretchr/testify/require"

schema "go.opentelemetry.io/otel/schema/v1.1"

)

func TestRender(t *testing.T) {

want, err := os.ReadFile("./testdata/transforms.go")

require.NoError(t, err)

schemaF, err := os.Open("./testdata/schema.yaml")

require.NoError(t, err)

defer schemaF.Close()

s, err := schema.Parse(schemaF)

require.NoError(t, err)

e, err := entries(s)

require.NoError(t, err)

var got bytes.Buffer

require.NoError(t, render(&got, e))

assert.Equal(t, string(want), got.String())

}

pellared · 2023-09-19T11:56:51Z

sdk/resource/internal/schema/generate/main.go

+	"flag"
+	"log"
+
+	sUtil "go.opentelemetry.io/otel/schema/v1.1"


Can you change the alias to schema as suggested https://pkg.go.dev/go.opentelemetry.io/otel/schema

pellared · 2023-09-19T11:58:07Z

sdk/resource/internal/schema/generate/main.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main


Can you please add some package documentation that would describe what this tool is doing and why it was created.

pellared · 2023-09-19T12:05:19Z

sdk/resource/internal/schema/generate/schema/fetch.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main


Can you please add some package documentation that would describe what this tool is doing and why it was created.

Also am I correct the usage of it is not currently implemented? Shouldn't it be invoked as part of make semconv-generate?

I'll update RELEASING.md to include it as a manual step for now.

pellared · 2023-09-19T12:12:53Z

sdk/resource/internal/schema/generate/render/render.go

+		return err
+	}
+
+	src, err := format.Source(out.Bytes())


We are shadowing const src = "transforms.go.tmpl" here. Maybe we should rename the const?

pellared · 2023-09-19T12:17:50Z

sdk/resource/resource.go

+// prior to the merge using the OpenTelemetry schema resource section
+// (https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/schemas/file_format_v1.1.0.md#resources-section)


Update the tag

Make it an URL

Suggested change

// prior to the merge using the OpenTelemetry schema resource section

// (https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/schemas/file_format_v1.1.0.md#resources-section)

// prior to the merge using

// [the OpenTelemetry schema resource section](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.25.0/specification/schemas/file_format_v1.1.0.md#resources-section)

pellared · 2023-09-19T12:24:47Z

sdk/resource/resource.go

+// (https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/schemas/file_format_v1.1.0.md#resources-section)
+// for the translation.
+//
+// If either of the resources have an invalid SchemaURL (non-OpenTelemetry or


It would be good to call out that "newer" versions of SchemaURL are also not supported. E.g. this will not magically support https://opentelemetry.io/schemas/1.22.0 once https://github.com/open-telemetry/semantic-conventions is released. I am not sure how to describe it in a readable way...

Won't we upgrade and release support for v1.22.0 when it is released?

We will. My point is that somebody may not understand that the support is "compiled"

Something like

If either of the resources have an unknown SchemaURL (only specific versions of OpenTelemetry schemas are supported), [...]

pellared · 2023-09-19T12:26:07Z

sdk/resource/resource_test.go

+			name: "BothUnknownSchemas",
+			a:    resource.NewWithAttributes("https://localhost/1", kv41),
+			b:    resource.NewWithAttributes("https://localhost/2", kv42),
+		},


How about adding this scenario

Suggested change

},

},

{

name: "NotSupportedOTelSchema",

a: resource.NewWithAttributes("https://opentelemetry.io/schemas/1.999.0", kv41),

b: resource.NewWithAttributes("https://opentelemetry.io/schemas/1.0.0", kv42),

},

Also I think in such scenario we should either:

a) simply return error that this semconv version is not supported and that the user can try using a new version of the OTel Go SDK
b) prefer all "known" translations (current implementation) and additionally return the error - it should be possible for the user to determine this error type so he could ignore the error returned in this scenario

jufemaiz · 2023-10-17T00:29:03Z

👀

MrAlias · 2024-01-31T01:03:09Z

Based on the resource merge specification:

The resulting resource MUST have all attributes that are on any of the two input resources. If a key exists on both the old and updating resource, the value of the updating resource MUST be picked (even if the updated value is empty).

Doing anything other than preserving all the keys provided to merge will be non-compliant.

pellared · 2024-01-31T08:15:19Z

Doing anything other than preserving all the keys provided to merge will be non-compliant.

I agree. I think that we can consider enhancing the specification so that (examples, brainstorming):

Merge can do transformations
Add an optional parameter to Merge that instructs to additionally perform resource transformations
Add new operation e.g. Transform that takes a resource and target scheme URL that tries to apply transformations for given resource.

MrAlias added 3 commits September 12, 2023 14:10

PoC

e7a31c8

Remove unneeded cmp

beed81a

Remove converter_test.go

a169b2b

MrAlias added the area:resources Part of OpenTelemetry resources label Sep 12, 2023

MrAlias and others added 7 commits September 12, 2023 14:20

Fix existing tests

519b76d

Merge branch 'main' into resource-conv

66b131d

Merge branch 'main' into resource-conv

27d8e93

Fix lint

0ec6659

Add changes to changelog

b652f10

Run go mod tidy and crosslink

8030dc5

Remove Downgrade func

020f5fa

MrAlias and others added 13 commits September 13, 2023 12:59

Merge converter.go into resource.go

4d4ae1c

Remove unused UnrenameFunc method

cf5f257

Add tests for Merge of invalid schema URLs

a0f1e4a

Add remote generation

8521044

Rename gen to generate

92f87a6

Fix lint

c935bd6

Restructure generate to add render test

d2ba722

Move render data to testdata dir

258ad94

Check error from render in test

876d2f8

Expand coverage of render test

b31c3ff

Regenerate schema.go

bd2136a

Run lint fixes

c214b69

Merge branch 'main' into resource-conv

aae49d7

MrAlias marked this pull request as ready for review September 14, 2023 14:52

MrAlias requested review from Aneurysm9, evantorrie, XSAM and dashpole as code owners September 14, 2023 14:52

MrAlias added 14 commits September 15, 2023 10:29

Rename convert*.go to schema*.go

a4d5347

Comment Version func

a1e11c6

Fix lint

e28199e

Merge generate/cmd into just cmd.go

1634182

Revert to static schema parsing

729f9a1

There is only one schema needed now. Keep it in code.

Embed schema in generate cmd

5fd0d7e

Rename cmd to render

c7985bf

Rename render.Run to render.Render

d55508e

Set schemaURL in merge of upgrade

b696cdd

Update New tests to expect resource upgrade in detectors

29103d9

Run linter

fcfc72a

Refactor resource merge tests

0be6881

Add suggested tests from review.

Run lint

2dd287d

Use semconv pkg for schema URL in fetch.go

f70951d

MrAlias force-pushed the resource-conv branch from 2088cee to f70951d Compare September 15, 2023 19:40

Remove error return from Upgrade

c9662ae

MrAlias marked this pull request as ready for review September 15, 2023 19:54

Merge branch 'main' into resource-conv

74e8197

pellared reviewed Sep 19, 2023

View reviewed changes

pellared mentioned this pull request Dec 5, 2023

BAD API DESIGN: cannot merge resource due to conflicting Schema URL #4476

Closed

pellared changed the title ~~Add schema translation functionality to resource creation and merge~~ Add schema transformation functionality to resource creation and merge Dec 5, 2023

pellared marked this pull request as draft December 12, 2023 09:33

pellared mentioned this pull request Dec 12, 2023

Upgrade all use of go.opentelemetry.io/otel/semconv to use v1.24.0 #4753

Closed

pellared mentioned this pull request Jan 22, 2024

example/dice: Use resource.NewSchemaless instead of resource.NewWithAttributes #4846

Closed

MrAlias closed this Jan 31, 2024

MrAlias mentioned this pull request Jan 31, 2024

Flatten schema pkg #4538

Closed

pellared mentioned this pull request Jan 31, 2024

Return merged Resource on schema conflict #4876

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add schema transformation functionality to resource creation and merge #4503

Add schema transformation functionality to resource creation and merge #4503

MrAlias commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

pellared Sep 19, 2023

MrAlias Sep 19, 2023

jufemaiz Sep 20, 2023

MrAlias Sep 21, 2023

pellared Sep 19, 2023

pellared Sep 19, 2023

pellared Sep 19, 2023

pellared Sep 19, 2023

MrAlias Sep 19, 2023

pellared Sep 19, 2023

pellared Sep 19, 2023 •

edited

Loading

pellared Sep 19, 2023

MrAlias Sep 19, 2023

pellared Sep 19, 2023

pellared Sep 19, 2023 •

edited

Loading

pellared Sep 19, 2023 •

edited

Loading

jufemaiz commented Oct 17, 2023

MrAlias commented Jan 31, 2024

pellared commented Jan 31, 2024

		// prior to the merge using the OpenTelemetry schema resource section
		// (https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/schemas/file_format_v1.1.0.md#resources-section)

-		},
+		},
+		{
+			name: "NotSupportedOTelSchema",
+			a:    resource.NewWithAttributes("https://opentelemetry.io/schemas/1.999.0", kv41),
+			b:    resource.NewWithAttributes("https://opentelemetry.io/schemas/1.0.0", kv42),
+		},

Add schema transformation functionality to resource creation and merge #4503

Add schema transformation functionality to resource creation and merge #4503

Conversation

MrAlias commented Sep 12, 2023 • edited Loading

Design

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

pellared Sep 19, 2023 • 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

pellared Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

pellared Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

jufemaiz commented Oct 17, 2023

MrAlias commented Jan 31, 2024

pellared commented Jan 31, 2024

MrAlias commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

pellared Sep 19, 2023 •

edited

Loading

pellared Sep 19, 2023 •

edited

Loading

pellared Sep 19, 2023 •

edited

Loading