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/add resource builder #2322

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

Conversation

pitoniak32
Copy link
Contributor

@pitoniak32 pitoniak32 commented Nov 22, 2024

fixes #2320

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(vec![
        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)

@pitoniak32 pitoniak32 requested a review from a team as a code owner November 22, 2024 01:30
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 89.38053% with 12 lines in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (3df15a1) to head (67e7c98).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-sdk/src/resource/builder.rs 94.2% 6 Missing ⚠️
opentelemetry-sdk/src/resource/mod.rs 33.3% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2322     +/-   ##
=======================================
- Coverage   79.4%   79.4%   -0.1%     
=======================================
  Files        123     124      +1     
  Lines      21485   21679    +194     
=======================================
+ Hits       17068   17214    +146     
- Misses      4417    4465     +48     

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

@pitoniak32 pitoniak32 force-pushed the feat/add-resource-builder branch from fdd9fd1 to b0bbed5 Compare November 22, 2024 01:40
@pitoniak32
Copy link
Contributor Author

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
Copy link
Contributor

utpilla commented Nov 23, 2024

@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
Copy link
Contributor Author

@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

@pitoniak32 pitoniak32 force-pushed the feat/add-resource-builder branch from f08a237 to afb8ba4 Compare November 23, 2024 23:46
}

/// Add a [KeyValue] to the resource.
pub fn with_key_value(self, kv: KeyValue) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

interesting why this does not need mut self...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😅

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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])
}

@pitoniak32 pitoniak32 force-pushed the feat/add-resource-builder branch 2 times, most recently from 5976ef0 to e501d4f Compare November 24, 2024 16:02
hdost
hdost previously requested changes Nov 26, 2024
Copy link
Contributor

@hdost hdost 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 the contribution 😊

opentelemetry-sdk/src/resource/builder.rs Outdated Show resolved Hide resolved
opentelemetry-sdk/src/resource/builder.rs Outdated Show resolved Hide resolved
@lalitb lalitb added this to the 0.27.1 milestone Nov 26, 2024
@pitoniak32 pitoniak32 force-pushed the feat/add-resource-builder branch from 9fca93a to 155f875 Compare December 1, 2024 20:29
}

/// Add multiple [KeyValue]s to the resource.
pub fn with_key_values<T: IntoIterator<Item = KeyValue>>(mut self, kvs: T) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this

self.resource = self.resource.merge(&Resource::new(kvs));
self
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@lalitb
Copy link
Member

lalitb commented Dec 2, 2024

/// 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:

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

@cijothomas cijothomas removed this from the 0.27.1 milestone Dec 2, 2024

impl ResourceBuilder {
/// Create ResourceBuilder with [Resource::empty()].
pub fn new() -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

is this needed, given Resource already expose public methods to create a ResourceBuilder?

Copy link
Member

Choose a reason for hiding this comment

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

wondering what is the best option?
Should users have the ability to get Builder instance from Builder itself, or via the thing it is ultimately building or both? We need to review rest of the codebase to ensure we do this consistently across.

I think its best to offer just one way to create a builder, and that must be via a method on the thing we are ultimately building.
so Resource::builder().....build() - only this should be required.
and not ResourceBuilder::new()....build().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the others are hiding the new method, I will update this, I agree with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the reason that was public is to allow the creation of an empty resource vs a default resource. In this case would it make more sense to make it pub(super)? so the resource is still able to use it? Or only allow starting from an empty resource, and then expose a with_default()?

@cijothomas cijothomas dismissed hdost’s stale review December 9, 2024 23:36

Changes done. Please re-review.

@cijothomas cijothomas added this to the 0.28.0 milestone Dec 10, 2024
/// If you want to start from a [Resource::default()] see [Resource::builder_default()].
///
/// Starts with a [Resource::empty()].
pub fn builder() -> ResourceBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

I am still debating about this.. It maybe better to name this builder_empty, to make it very obvious that this starts with an empty Resource, and not with OTel SDK defaults?

resource: Resource,
}

impl Default for ResourceBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

we can remove the Default implementation for the builder, so as to force users to always go via Resource to get hold of a builder.

"service.name",
"metrics-advanced-example",
)]))
.with_resource(resource)
Copy link
Member

Choose a reason for hiding this comment

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

  1. If user never called with_resource.- use SDK defaults.
  2. with_resource(Resource::builder().with..with...build()) - Start with SDK defaults, and merge the rest.
  3. with_resource(Resource::builder_empty().with..with...build()) - Start with empty, and merge the rest.
  4. provider.append_resource(..) - merges with existing Resource(s)

^ Does this look the right approach @open-telemetry/rust-approvers please share your thoughts, this is important to get closure soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

After some more thinking into this, I believe there mainly two use-cases here:

  1. Set resource to something very specific (remove default resources)
  2. Append more resources to the existing ones

I believe a simple solution here could be:

  • Introduce an API named set_resource(resource) which will replace the provider's resource with the value in the method argument. This would cover the 1st use case.
  • Use the existing with_resource(resource) API (or rename it to append_resource(resource) to allow users to append/merge resources with the existing provider resource. The existing resource would either be the default ones or something else if the user called set_resource(resource). This would cover the 2nd use case.

Copy link
Member

@lalitb lalitb Dec 11, 2024

Choose a reason for hiding this comment

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

Do we have the use-case where resource would be updated/merged in Provider after it's creation. As I understand, the user needs to ensure that it creates the resource object by merging all the default/custom/required detectors before creating the Provider. And in that case, set_resource(), add_resource() and merge_resource() on Provider is not required. So,

These looks fine:

  • If user never called with_resource.- use SDK defaults.

This is also fine:

  • with_resource(Resource::builder().with..with...build()) - Start with SDK defaults, and merge the rest.
  • with_resource(Resource::builder_empty().with..with...build()) - Start with empty, and merge the rest.

Or maybe another option could be:
Resource::builder(start_with_default: bool) -> ResourceBuilder
i.e,

  • with_resource(Resource::builder(start_with_default: true).with..with...build()) - Start with SDK defaults, and merge the rest.
  • with_resource(Resource::builder(start_with_default: false).with..with...build()) - Start with empty resource, and merge the rest.
    And ResourceBuilder::new() and ResourceBuilder::default() can be made private/removed.

But NOT this, as I don't see it's use-case:

  • provider.append_resource(..) - merges with existing Resource(s).

the user should instead do:

  let merged_resource = resource.merge(to_be_merged);
  let provider = Provider::builder().with_resource(merged_resource)... .build();

@@ -61,7 +63,29 @@ impl Default for Resource {
}
}

impl From<ResourceBuilder> for Resource {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to implement this conversion. I believe the intuitive thing to do is call build() on a builder type and not into().

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.

Improve Resource related public APIs
6 participants