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

Add Schema URL support to Resource #1938

Merged

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented May 21, 2021

This implements specification requirement:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#resource-creation

  • Change resource.NewWithAttributes to require a schema URL. The old function
    is still available as resource.NewSchemaless. This is a breaking change.
    We want to encourage using schema URL and make it a conscious choice to have a
    resource without schema.

  • resource.Merge merges schema URLs according to the spec.

  • Several builtin resource detectors now correctly populate the schema URL.

@tigrannajaryan
Copy link
Member Author

@MrAlias @Aneurysm9 Please have a look.

This PR implements the Resource portion of schema URL. It imports the current semconv package. Once we agree on #1933 this will need to be changed to import a specific semconv package version.

@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #1938 (41f780b) into main (0827aa6) will increase coverage by 0.0%.
The diff coverage is 87.6%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1938   +/-   ##
=====================================
  Coverage   77.1%   77.2%           
=====================================
  Files        161     161           
  Lines       8554    8595   +41     
=====================================
+ Hits        6602    6637   +35     
- Misses      1696    1699    +3     
- Partials     256     259    +3     
Impacted Files Coverage Δ
exporters/otlp/internal/transform/span.go 98.1% <ø> (ø)
...ters/otlp/otlptrace/internal/otlptracetest/data.go 0.0% <0.0%> (ø)
.../otlp/otlptrace/internal/otlptracetest/otlptest.go 0.0% <0.0%> (ø)
sdk/metric/controller/basic/config.go 84.6% <33.3%> (-15.4%) ⬇️
sdk/trace/provider.go 83.0% <50.0%> (-1.6%) ⬇️
sdk/resource/env.go 92.5% <75.0%> (-7.5%) ⬇️
bridge/opencensus/exporter.go 100.0% <100.0%> (ø)
exporters/otlp/internal/otlptest/data.go 88.0% <100.0%> (ø)
exporters/otlp/internal/otlptest/otlptest.go 76.4% <100.0%> (ø)
exporters/otlp/internal/transform/metric.go 77.1% <100.0%> (+0.1%) ⬆️
... and 7 more

sdk/resource/resource.go Outdated Show resolved Hide resolved
semconv/schema.go Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/resource-schema-url branch 3 times, most recently from e689c86 to d896358 Compare May 28, 2021 18:08
@tigrannajaryan
Copy link
Member Author

@MrAlias @Aneurysm9 Assuming we are OK with NewWithAttributes function renaming this is ready for review.

@tigrannajaryan tigrannajaryan marked this pull request as ready for review May 28, 2021 18:14
@tigrannajaryan tigrannajaryan changed the title [WIP] Add Schema URL support to Resource Add Schema URL support to Resource May 28, 2021
@tigrannajaryan
Copy link
Member Author

Tests are unstable and keep failing. I do not think it is related to my changes in any way (e.g. TestNewBatchSpanProcessorWithOptions failed).

sdk/resource/auto.go Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/resource-schema-url branch from d896358 to 9fe13b2 Compare June 3, 2021 13:57
@MrAlias MrAlias added this to In progress in OpenTelemetry Go RC via automation Jun 3, 2021
@MrAlias MrAlias added this to the RC1 milestone Jun 3, 2021
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Is the end goal to require all resource be linked to a schema? If so, I'm hesitant to release a way to create a resource without one (NewSchemaless).

I have some suggestions as to how we can make this happen, but want to make sure I understand the end goal first.

CHANGELOG.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member Author

Is the end goal to require all resource be linked to a schema? If so, I'm hesitant to release a way to create a resource without one (NewSchemaless).

I think we can't force that yet. The schema should be optional. The SDK will create resources with the right schema out of the box, but I don't think we are ready to declare schemaless resources invalid.

@MrAlias
Copy link
Contributor

MrAlias commented Jun 4, 2021

I don't think we are ready to declare schemaless resources invalid.

Is this something we would declare in v2?

@tigrannajaryan
Copy link
Member Author

I don't think we are ready to declare schemaless resources invalid.

Is this something we would declare in v2?

I am not sure, probably not. I guess there will always be the cases when people want to "just send some resource" without putting much thought into what schema it follows.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/resource-schema-url branch from 9fe13b2 to 6782b74 Compare June 4, 2021 16:50
@Aneurysm9
Copy link
Member

I don't think we are ready to declare schemaless resources invalid.

Is this something we would declare in v2?

I am not sure, probably not. I guess there will always be the cases when people want to "just send some resource" without putting much thought into what schema it follows.

Agreed. Users may put arbitrary keys/values into resources that have no relation to any defined schema and I'm not sure we should stop them.

This implements specification requirement:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#resource-creation

- Changes `resource.NewWithAttributes` to require a schema URL. The old function
  is still available as `resource.NewSchemaless`. This is a breaking change.
  We want to encourage using schema URL and make it a conscious choice to have a
  resource without schema.

- Merge schema URLs acccording to the spec in resource.Merge.

- Several builtin resource detectors now correctly populate the schema URL.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/resource-schema-url branch from 6782b74 to 6c30e22 Compare June 7, 2021 20:22
@tigrannajaryan
Copy link
Member Author

test-coverage failed for reasons unrelated to my change: https://github.com/open-telemetry/opentelemetry-go/runs/2767758746?check_suite_focus=true
I don't understand what is wrong from the job output.

I will rerun for now.

@tigrannajaryan
Copy link
Member Author

@Aneurysm9 @MrAlias can we merge or you want to have another look?

OpenTelemetry Go RC automation moved this from In progress to Reviewer approved Jun 8, 2021
@MrAlias MrAlias merged commit 46d9687 into open-telemetry:main Jun 8, 2021
OpenTelemetry Go RC automation moved this from Reviewer approved to Done Jun 8, 2021
@tigrannajaryan tigrannajaryan deleted the feature/tigran/resource-schema-url branch June 8, 2021 17:23
@Aneurysm9 Aneurysm9 mentioned this pull request Jun 17, 2021
@tonglil
Copy link
Contributor

tonglil commented Nov 11, 2021

Just an end user chiming in here when trying to upgrade my launcher from 0.20 to 1.1, and I'm not understanding what schemas and their URLs are used for?

I wrote a detector and just trying to create resource.NewWithAttributes(a...) but looks like I have to maintain some kind of schema and expose it on a server somewhere?

For example, if I was doing something like:

// Detect detects from the environment ENV and VERSION.
func (Environment) Detect(ctx context.Context) (*resource.Resource, error) {
	a := []attribute.KeyValue{
		semconv.DeploymentEnvironmentKey.String(os.Getenv("ENV")),
		semconv.ServiceNameKey.String(os.Getenv("SERVICE_NAME")),
		semconv.ServiceVersionKey.String(os.Getenv("SERVICE_VERSION")),
	}

	return resource.NewWithAttributes(a...), nil
}

Am I suppose to write my own schema, or does this fit the 1.4.0 semconv schema because I'm using semconv.* keys?

The entire versioning semconv and schema thing isn't very well explained anywhere (and the spec is too convoluted to understand).

Maybe I'm obtuse, or OTel is not user friendly, but I'm likely going to use NewSchemaless for the mean time until this is better documented.

@tigrannajaryan
Copy link
Member Author

Am I suppose to write my own schema, or does this fit the 1.4.0 semconv schema because I'm using semconv.* keys?

No, you don't need to write your own schema and don't need to publish it on a server. If you are using a semconv package then you are using OpenTelemetry schema. OpenTelemetry schema is already published at https://opentelemetry.io/schemas/ (e.g. https://opentelemetry.io/schemas/1.7.0).

All you need to do is pass the SchemaURL from the semconv package that you are importing, e.g.:

// Detect detects from the environment ENV and VERSION.
func (Environment) Detect(ctx context.Context) (*resource.Resource, error) {
	a := []attribute.KeyValue{
		semconv.DeploymentEnvironmentKey.String(os.Getenv("ENV")),
		semconv.ServiceNameKey.String(os.Getenv("SERVICE_NAME")),
		semconv.ServiceVersionKey.String(os.Getenv("SERVICE_VERSION")),
	}

	return resource.NewWithAttributes(semconv.SchemaURL, a...), nil
}

This correctly sets the schema URL to match the semantic conventions that you are using for the attributes.

The only case that you want to create your own schema and publish it on a server is when you are NOT using OpenTelemetry's semantic conventions and are inventing your own conventions for everything. For the vast majority of users this is unnecessary and undesirable.

@tonglil
Copy link
Contributor

tonglil commented Nov 19, 2021

I see, thanks for elaborating.
I hope this meaning/requirement can be more clearly documented, perhaps particularly in the code/godocs.

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

Successfully merging this pull request may close these issues.

None yet

5 participants