-
Notifications
You must be signed in to change notification settings - Fork 463
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
feat(common): add schema_url to resource. #775
Conversation
Codecov Report
@@ Coverage Diff @@
## main #775 +/- ##
=======================================
+ Coverage 69.8% 69.9% +0.1%
=======================================
Files 109 109
Lines 8912 8968 +56
=======================================
+ Hits 6226 6277 +51
- Misses 2686 2691 +5
Continue to review full report at Codecov.
|
/// Return the [schema url] of the resource. If the resource does not have a schema url, return empty string. | ||
/// | ||
/// [schema url]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.9.0/specification/schemas/overview.md#schema-url | ||
pub fn schema_url(&self) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have this be Option<String>
? or maybe more optimally Option<&str>
? schema_url being a valid URL or None
may be less error prone than having it be a valid URL or an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable 👍 Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, could we get &str
here? or does that cause issues
Part of #711.
Note that when merging two resources:
For the last cases, merging error. The current implementation set the
schema_url
of the result toNone
.Block on #776