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

feat(common): add schema_url to resource. #775

Merged
merged 7 commits into from Apr 17, 2022

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Apr 7, 2022

Part of #711.

Note that when merging two resources:

If the old resource's Schema URL is empty then the resulting resource's Schema URL will be set to the Schema URL of the updating resource,
Else if the updating resource's Schema URL is empty then the resulting resource's Schema URL will be set to the Schema URL of the old resource,
Else if the Schema URLs of the old and updating resources are the same then that will be the Schema URL of the resulting resource,
Else this is a merging error (this is the case when the Schema URL of the old and updating resources are not empty and are different). The resulting resource is undefined, and its contents are implementation-specific.

For the last cases, merging error. The current implementation set the schema_url of the result to None.

Block on #776

@TommyCpp TommyCpp marked this pull request as ready for review April 10, 2022 02:57
@TommyCpp TommyCpp requested a review from a team as a code owner April 10, 2022 02:57
@codecov
Copy link

codecov bot commented Apr 10, 2022

Codecov Report

Merging #775 (79dca72) into main (a474268) will increase coverage by 0.1%.
The diff coverage is 93.3%.

@@           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     
Impacted Files Coverage Δ
opentelemetry-sdk/src/resource/mod.rs 92.5% <93.3%> (+<0.1%) ⬆️
opentelemetry-sdk/src/metrics/controllers/push.rs 80.0% <0.0%> (-3.4%) ⬇️
opentelemetry-datadog/src/exporter/mod.rs 10.6% <0.0%> (-0.1%) ⬇️
opentelemetry-otlp/src/span.rs 0.0% <0.0%> (ø)
opentelemetry-sdk/src/trace/span.rs 93.4% <0.0%> (ø)
opentelemetry-sdk/src/trace/tracer.rs 90.7% <0.0%> (+<0.1%) ⬆️
opentelemetry-zipkin/src/exporter/model/span.rs 94.0% <0.0%> (+0.1%) ⬆️
opentelemetry-dynatrace/src/transform/metrics.rs 83.6% <0.0%> (+0.1%) ⬆️
opentelemetry-datadog/src/exporter/model/mod.rs 74.5% <0.0%> (+0.4%) ⬆️
opentelemetry-sdk/src/testing/trace.rs 67.5% <0.0%> (+0.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a474268...79dca72. Read the comment docs.

/// 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 {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable 👍 Updated

Copy link
Member

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

@TommyCpp TommyCpp merged commit fdf6401 into open-telemetry:main Apr 17, 2022
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.

None yet

2 participants