open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.22k stars 1.05k forks source link

resource.Default()/etc is a footgun #3769

Closed ash2k closed 11 months ago

ash2k commented 1 year ago

Problem Statement

https://github.com/open-telemetry/opentelemetry-go/blob/69d09462dbd8497416857dc18542232857d152c1/example/fib/main.go#L45-L53

Code above gives an example of how to use resource.Default(). There is a problem with this code though - if you update otel dependencies you may get a cannot merge resource due to conflicting Schema URL error when running the code. This happens if otel SDK starts using a different version of the schema to what it used to use and what is used in the example/user code. I.e. schemas used to match but do not after SDK change. From user's perspective it all worked, but suddenly starts failing at runtime.

This is especially problematic if resource merging is conditional e.g. when it's only done when tracing is enabled. I've just had a situation where I bumped the otel dependencies, all tests passed, I cut a release and then it blew up when deployed to a canary environment where tracing is actually enabled.

It's not only resource.Default(), a few other methods in the resource package have the same issue.

Proposed Solution

Not sure. Approaches that are better than failing at runtime:

Alternatives

Prior Art

Additional Context

ash2k commented 1 year ago

Just added a unit test to my codebase and it fails:

import (
    "go.opentelemetry.io/otel/sdk/resource"
    semconv "go.opentelemetry.io/otel/semconv/v1.12.0"
)

func constructResource() (*resource.Resource, error) {
    return resource.Merge(
        resource.Default(),
        resource.NewWithAttributes(
            semconv.SchemaURL,
            semconv.ServiceNameKey.String("some name"),
            semconv.ServiceVersionKey.String("some ver"),
        ),
    )
}

func TestConstructResource(t *testing.T) {
    _, err := constructResource()
    require.NoError(t, err)
}
            Error:          Received unexpected error:
                            cannot merge resource due to conflicting Schema URL
            Test:           TestConstructResource
--- FAIL: TestConstructResource (0.00s)

Fix is to change import to semconv "go.opentelemetry.io/otel/semconv/v1.17.0". This is not a good UX :)

ash2k commented 1 year ago

Perhaps those methods can be moved into the "versioned" packages? They can use the same underlying types, but a matching schema URL from that particular package. In the future more attributes can be added to new scema versions and having e.g. Default() in a versioned package expose that attribute only for a certain version would make sense. Put differently, today I cannot use those helper methods with older schema versions.

Aneurysm9 commented 1 year ago

resource.Merge() does not have the schema translation capabilities it needs to make this work as intended. The schema module provides the schema parsing capabilities required to implement this. There is a collector schemaprocessor that is under construction that can demonstrate the use of that module. That said, implementing translation in resource.Merge() may be in violation of the resource specification.

MrAlias commented 1 year ago

Here's a new repo I created to handle schema translations: https://github.com/MrAlias/otel-schema-utils

Only thing supported currently is resource translations, but out of this it does provide a resource merge functionality that is schema URL aware: https://pkg.go.dev/github.com/MrAlias/otel-schema-utils@v0.1.0-alpha/schema#Converter.MergeResources

pellared commented 11 months ago

Closing as duplicate of https://github.com/open-telemetry/opentelemetry-go/issues/2341