Feat/add resource builder

[pitoniak32]

fixes #2320

Migration Guide

Creating a Resource with default detectors:

// old
let resource = Resource::default()
    .with_attributes([
        KeyValue::new("service.name", "example_service"),
        KeyValue::new("test1", "test_value"),
        KeyValue::new("test1", "test_value1"),
        KeyValue::new("test2", "test_value2"),
    ]);

// new
let resource = Resource::builder()
    .with_service_name("example_service")
    .with_attributes([
        KeyValue::new("test1", "test_value")
        KeyValue::new("test1", "test_value1"),
        KeyValue::new("test2", "test_value2"),
    ])
    .build();

Creating an empty Resource:

// old
let resource = Resource::empty();

// new
let resource = Resource::builder_empty().build();

Changes

• Add ResourceBuilder to more easily create resources.

example usage:

let resource = Resource::builder()
    .with_service_name("gifting_service")
    .with_detector(Box::new(EnvResourceDetector::new()))
    .with_attribute(KeyValue::new("test1", "test_value"))
    .with_attributes([
        KeyValue::new("test1", "test_value1"),
        KeyValue::new("test2", "test_value2"),
    ])
    .build();

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: Patch coverage is 93.65079% with 16 lines in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (15d69b1) to head (c020eec).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-zipkin/src/exporter/mod.rs	0.0%	13 Missing ⚠️
opentelemetry-sdk/src/resource/env.rs	97.2%	1 Missing ⚠️
opentelemetry-stdout/src/logs/exporter.rs	0.0%	1 Missing ⚠️
opentelemetry-stdout/src/trace/exporter.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2322     +/-   ##
=======================================
+ Coverage   79.3%   79.4%   +0.1%     
=======================================
  Files        122     122             
  Lines      21588   21708    +120     
=======================================
+ Hits       17138   17255    +117     
- Misses      4450    4453      +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pitoniak32]

I still need to expose the new builder to the API, and would like feedback on #2324 before going further since the changes are related :)

[utpilla]

@pitoniak32 Could you split this into two PRs? Have one PR just for removing the timeout argument and one for resource builder. That way we can merge the timeout related PR while we review the builder related changes.

[pitoniak32]

@pitoniak32 Could you split this into two PRs? Have one PR just for removing the timeout argument and one for resource builder. That way we can merge the timeout related PR while we review the builder related changes.

split those changes into: #2332

[cijothomas]

interesting why this does not need mut self...

[pitoniak32]

It might be since the with_detectors consumes the builder and its not a &mut reference? That's just a guess I'm actually not sure about that 😅

[lalitb]

with_key_value doesn’t need mut self because it doesn’t directly modify the builder; it delegates to with_key_values, which takes mut self to perform the actual mutation.

[pitoniak32]

I was also thinking of doing something like this for with_key_value, I'm wondering what others think?

let resource = Resource::builder()
    .with_key_value(KeyValue::new("test1", "test_value"))
    .build();

// vs

let resource = Resource::builder()
    .with_key_value("test1", "test_value")
    .build();

/// Add a [KeyValue] to the resource.
pub fn with_key_value<K, V>(self, key: K, value: V) -> Self
where
    K: Into<Key>,
    V: Into<Value>,
{
    self.with_key_values(vec![KeyValue::new(key, value)])
}

currently its:

/// Add a [KeyValue] to the resource.
pub fn with_key_value(self, kv: KeyValue) -> Self {
    self.with_key_values(vec![kv])
}

[hdost]

Thank you for the contribution 😊

[lalitb]

should the method be named with_attribute and with_attributes - Just that the spec uses the term "attribute" consistently when describing resource data.

[cijothomas]

+1

[pitoniak32]

fixed this

[lalitb]

should there also be a method with_schema_url to add schema url ?

[pitoniak32]

How would we like the behavior of the with_schema_url to act? Do we want to follow the rules that from_schema_url has? Or override the current one with the specified one, or maybe use TypeState pattern to only allow the schema_url to be set once?

[pitoniak32]

added this, would love to hear feedback on the implementation!

[lalitb]

/// Add a [KeyValue] to the resource.
pub fn with_key_value<K, V>(self, key: K, value: V) -> Self
where
K: Into,
V: Into,
{
self.with_key_values(vec![KeyValue::new(key, value)])
}

I vote for this, it's similar to what is followed for LogRecord::add_attribute:

opentelemetry-rust/opentelemetry-sdk/src/logs/record.rs

Lines 98 to 102 in a3c469b

	fn add_attribute<K, V>(&mut self, key: K, value: V)
	where
	K: Into<Key>,
	V: Into<AnyValue>,
	{

Changes done. Please re-review.

[cijothomas]

LGTM.
I think we can keep few methods on Resource like new() as pub(mod) to reduce the amount of internal changes.

Please update changelog to callout the breaking change/migration snippets.

Thanks for patiently working through this!

[utpilla]

Looks good overall! I left some non-blocking suggestions.

Thanks @pitoniak32 for helping with this refactoring!

[lalitb]

LGTM! This looks great. Thank you for making all the updates and for your patience in addressing the comments

ps. Integration test is failing.

[pitoniak32]

LGTM! This looks great. Thank you for making all the updates and for your patience in addressing the comments

ps. Integration test is failing.

does it make sense to keep using an empty resource for integration tests, or should it get updated to have the default detectors?

[cijothomas]

LGTM! This looks great. Thank you for making all the updates and for your patience in addressing the comments
ps. Integration test is failing.

does it make sense to keep using an empty resource for integration tests, or should it get updated to have the default detectors?

either is fine! Do what is easiest for this PR.