Skip to content

feat(opentelemetry-resources): add schema url #5753

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

c-ehrlich
Copy link

@c-ehrlich c-ehrlich commented Jun 12, 2025

Which problem is this PR solving?

Resolves #4182

Short description of the changes

The JavaScript SDK, unlike most others, currently does not give the ability to provide a Schema URL to a resource. See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#resource-creation

This PR implements the ability to provide it.

There was a previous attempt at this by another contributor, which was closed: #5070

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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

npm run test

  • should create resource with schema URL
  • should create resource without schema URL
  • should merge resources with schema URL priority given to other resource
  • should retain schema URL from base resource when other has no schema URL
  • should retain schema URL from the resource that has it when merging
  • should have empty schema URL when merging resources with no schema URL
  • should handle merging with empty string schema URLs
  • should maintain backward compatibility - getSchemaUrl is optional
  • should work with async attributes and schema URLs

Checklist:

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

@c-ehrlich c-ehrlich marked this pull request as ready for review June 12, 2025 14:22
@c-ehrlich c-ehrlich requested a review from a team as a code owner June 12, 2025 14:22
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

thank you for working on this 🙂

@c-ehrlich c-ehrlich requested a review from pichlermarc June 16, 2025 06:17
Copy link

codecov bot commented Jun 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.02%. Comparing base (f2cfd13) to head (9046e4d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5753      +/-   ##
==========================================
+ Coverage   95.01%   95.02%   +0.01%     
==========================================
  Files         303      303              
  Lines        7957     7984      +27     
  Branches     1610     1618       +8     
==========================================
+ Hits         7560     7587      +27     
  Misses        397      397              
Files with missing lines Coverage Δ
...l/packages/otlp-transformer/src/common/internal.ts 100.00% <100.00%> (ø)
...tal/packages/otlp-transformer/src/logs/internal.ts 100.00% <100.00%> (ø)
.../packages/otlp-transformer/src/metrics/internal.ts 100.00% <100.00%> (ø)
...al/packages/otlp-transformer/src/trace/internal.ts 98.43% <100.00%> (+0.02%) ⬆️
...ckages/opentelemetry-resources/src/ResourceImpl.ts 98.83% <100.00%> (+0.35%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

thanks for addressing the comments, overall this looks good already - just one small blocker left 🙂

Comment on lines +62 to +68

/**
* Get the schema URL for this resource.
*
* @returns the schema URL or undefined if not set
*/
getSchemaUrl?(): string | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer a readonly property, then use a getter in the ResourceImpl to enforce it, like we do with the other properties to keep it consistent.

cc @dyladan do you have a preference? (since you re-did resources in the latest major version)

assert.strictEqual(mergedResource.getSchemaUrl?.(), schemaUrl);
});

it('should retain schema URL from the resource that has it when merging', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you might have a duplicated test case between this and the next case.


warnStub.restore();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to add a negative test to ensure invalid schema URL formats are handled gracefully.

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

Successfully merging this pull request may close these issues.

Add Schema URL for resources
3 participants