feat(opentelemetry-resources): add schema url #5753
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5753 +/- ##
==========================================
+ Coverage 95.03% 95.05% +0.01%
==========================================
Files 306 306
Lines 7969 8001 +32
Branches 1611 1620 +9
==========================================
+ Hits 7573 7605 +32
Misses 396 396
🚀 New features to boost your workflow:
|
|
Looks like most things are resolved here. After another round of reviews I think this can merge. |
| if (schemaUrl === undefined || schemaUrl === '') { | ||
| return schemaUrl; | ||
| } | ||
|
|
||
| // Reject any ASCII C0 control (0x00–0x1F), SPACE (0x20) or DEL (0x7F) | ||
| // This ensures consistent behavior across Node.js and browser environments | ||
| // eslint-disable-next-line no-control-regex | ||
| if (/[\x00-\x20\x7F]/.test(schemaUrl)) { | ||
| diag.warn(INVALID_SCHEMA_URL_MESSAGE, schemaUrl); | ||
| return undefined; | ||
| } | ||
|
|
||
| try { | ||
| const url = new URL(schemaUrl); | ||
|
|
||
| if (url.protocol !== 'http:' && url.protocol !== 'https:') { | ||
| diag.warn(INVALID_SCHEMA_URL_MESSAGE, schemaUrl); | ||
| return undefined; | ||
| } | ||
|
|
||
| return schemaUrl; | ||
| } catch { | ||
| diag.warn(INVALID_SCHEMA_URL_MESSAGE, schemaUrl); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Hmm, do other SDK validate the schema URL?
I'm wondering if we'd be overly strict here.
There was a problem hiding this comment.
Was added as a response to this #5753 (comment)
What would you suggest? Maybe just a runtime type guard to ensure it's either a string or undefined, and fall back to undefined if not?
There was a problem hiding this comment.
Maybe just a runtime type guard to ensure it's either a string or undefined, and fall back to undefined if not?
Yes I think that works for me. In Java they don't validate the schemaUrl, they also don't do it in Python - looking at the spec there's no statement about having to validate schema URLs.
There was a problem hiding this comment.
Made the changes, let me know what you think :)
|
Thank you for your contribution @c-ehrlich! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
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
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 testChecklist: