feat(opentelemetry-resources): add schema url

[taylorhelene]

Which problem is this PR solving?

Add Schema URL for resources : #4182

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

I have added the schema URL to the Resource.ts and made the necessary changes to the IResource.ts. I have fixed the first two checkboxes and I am asking for clarification on the third because most of the exporters packages in the experimental folder are already exporting resources. I am trying to add a new feature request as per the issue request.

Fixes # (Schema URL can be added using the Resource constructor and Schema URL is handled according to the specification on Resource#merge)

Short description of the changes

I have added schemaUrl parameter, added a return function and merged schema URLs, handling conflicts

Type of change

Please delete options that are not relevant.

• [ x ] Bug fix (non-breaking change which fixes an issue)
• [ x ] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

I added some tests to the Resource.test.ts within the test suite.

• cd opentelemetry-js/packages/opentelemetry-resources

• npm run test

• should initialize resource with schema URL

• should merge resources with different schema URLs

• should retain the same schema URL when merging resources with identical URL

• should retain schema URL from the resource that has it when merging

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[pichlermarc]

Thanks for taking the time to work on this - there's a few comment regarding possible breaking changes that need to be addressed before we can move on. 🙂

Working on the Resource package is kind of tricky as it's essentially used everywhere. We need to be very sure that we don't introduce any breaking changes and there's lots of areas where this can happen.

[pichlermarc]

this is a stable interface, we can only add new optional properties here - this is likely why you had to adjust the tests (adding as Resource and as any)

[pichlermarc]

I think since IResource#getSchemaUrl() is defined, we can use

Suggested change

	const mergedSchemaUrl = this._mergeSchemaUrls(this._schemaUrl || '', (other as Resource)._schemaUrl || '');
	const mergedSchemaUrl = this._mergeSchemaUrls(this._schemaUrl || '', other.getSchemaUrl?.() || '');

and we can get rid of the back-cast. 🙂

[pichlermarc]

here we've already established that schema URL is a string (not undefined or null) though the type-system - why add null-checks? 🤔

[pichlermarc]

I think this would be better as a stand-alone utility function as it does not access anything on the instance.

[pichlermarc]

missing newline 🙂

Suggested change

	}
	}

[pichlermarc]

Suggested change

	private _schemaUrl?: string; // Added schemaUrl property
	private _schemaUrl?: string;

[pichlermarc]

Suggested change

	schemaUrl: string = '' // Added schemaUrl parameter
	schemaUrl: string = ''

[pichlermarc]

Suggested change

	this._schemaUrl = schemaUrl; // Store the schemaUrl
	this._schemaUrl = schemaUrl;

[pichlermarc]

Suggested change

	const debugStub = sinon.spy(diag, 'warn');
	const mergedResource = resource1.merge(resource2);
	assert.strictEqual(mergedResource.getSchemaUrl(), schemaUrl1);
	assert.ok(debugStub.calledWithMatch('Schema URLs differ. Using the original schema URL.'));
	const warnStub = sinon.spy(diag, 'warn');
	const mergedResource = resource1.merge(resource2);
	assert.strictEqual(mergedResource.getSchemaUrl(), schemaUrl1);
	assert.ok(warnStub.calledWithMatch('Schema URLs differ. Using the original schema URL.'));

[pichlermarc]

The comment above this detector still holds - if this needs to be modified, the changes in this PR are breaking.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.27%. Comparing base (7293e69) to head (ab3f9c6).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5070   +/-   ##
=======================================
  Coverage   93.26%   93.27%           
=======================================
  Files         317      317           
  Lines        8195     8203    +8     
  Branches     1641     1646    +5     
=======================================
+ Hits         7643     7651    +8     
  Misses        552      552           

Files with missing lines	Coverage Δ
packages/opentelemetry-resources/src/Resource.ts	100.00% <100.00%> (ø)

[pichlermarc]

Regarding the question about the OTLP exporters - this is now mostly related to @opentelemetry/otlp-transformer where the internal representation of Metrics/Spans/Logs is converted to OTLP (json or protobuf), that also includes the resource.

We have transformation code where we currently don't set the schema url (note that IResource there and IResource from @opentelemetry/resources are different types:

opentelemetry-js/experimental/packages/otlp-transformer/src/resource/internal.ts

Lines 20 to 25 in 7ed67f9

	export function createResource(resource: ISdkResource): IResource {
	return {
	attributes: toAttributes(resource.attributes),
	droppedAttributesCount: 0,
	};
	}

opentelemetry-js/experimental/packages/otlp-transformer/src/resource/types.ts

Lines 20 to 26 in 7ed67f9

	export interface IResource {
	/** Resource attributes */
	attributes: IKeyValue[];
	/** Resource droppedAttributesCount */
	droppedAttributesCount: number;
	}

so the schema url would be dropped on export. The last point I put on the issue was to ensure that we also add code for the transformation on the export, otherwise we'll hold it internally but we'd never export. 🙂

[taylorhelene]

I have added some more changes, can you please review them . Thank you.

[pichlermarc]

this is also an indicator that this is a breaking change.

[pichlermarc]

This is a breaking change - @opentelemetry/resources is a stable SDK package, and new properties added to interfaces must be optional.

[pichlermarc]

we need to address the comments from my previous review - otherwise we'll introduce breaking changes that will affect a large part of our userbase.

[pichlermarc]

(marking as changes requested)

[taylorhelene]

Okay, I'll work on the requested changes.

[taylorhelene]

I got stuck. Do I also change the tests where I had introduced the breaking changes?