Feat/add resource builder #2322

pitoniak32 · 2024-11-22T01:30:41Z

Changes

Add ResourceBuilder to more easily create resources.

example usage:

let resource = Resource::builder()
    .with_detector(Box::new(EnvResourceDetector::new()))
    .with_key_value(KeyValue::new("test1", "test_value"))
    .with_key_values(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)

codecov · 2024-11-22T01:34:26Z

Codecov Report

Attention: Patch coverage is 88.23529% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.5%. Comparing base (a3c469b) to head (c00aff4).

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/resource/builder.rs	91.9%	5 Missing ⚠️
opentelemetry-sdk/src/resource/mod.rs	50.0%	3 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2322   +/-   ##
=====================================
  Coverage   79.5%   79.5%           
=====================================
  Files        123     124    +1     
  Lines      21492   21560   +68     
=====================================
+ Hits       17096   17156   +60     
- Misses      4396    4404    +8

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

pitoniak32 · 2024-11-22T13:10:17Z

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 · 2024-11-23T20:40:32Z

@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 · 2024-11-23T23:45:15Z

@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 · 2024-11-24T15:29:10Z

opentelemetry-sdk/src/resource/builder.rs

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


interesting why this does not need mut self...

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 😅

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.

opentelemetry-sdk/src/resource/mod.rs

opentelemetry-sdk/src/resource/builder.rs

stress/src/throughput.rs

pitoniak32 · 2024-11-24T15:47:07Z

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

cijothomas · 2024-11-24T16:40:47Z

examples/metrics-advanced/src/main.rs

-            "service.name",
-            "metrics-advanced-example",
-        )]))
+        .with_resource(resource)


wondering if we can do with_servicename_resource("metrics-advanced-example") ? This will be the most common use case.
Or just Resource::builder().with_servicename("metrics-advanced-example")......build() ?

I like that, I was using the go implementation for Resource builder this weekend and they had something similar to this and it felt pretty nice

sResource, err := resource.New( ctx, resource.WithTelemetrySDK(), resource.WithAttributes(semconv.ServiceName("trace-export-service")), )

added the option of .with_service_name("name")

Hmm should we give special treatment to service name? I feel like it will open a can of worms that which attributes should get dedicate builder method. I think we should keep the API lean for now.

On the long term we can use dedicate trait to enrich the builder. Image something like

pub trait SemCov { fn with_service_name(&str); fn with_service_version(&str); ... // those method can probably generated in sem cov crate } impl SemCov for ResourceBuilder { fn with_service_name(&str) { ... // again we can generated those code in sem cov crate } } // main.rs import {ResourceBuilder, SemCov} let builder = ResourceBuilder::new().with_service_name("test")

So for people don't care about build size, they can opt in method for all attributes on resource builder by importing SemCov trait.

Sounds reasonable to me, I was kind of wondering the same thing where we would want to use the key from sem conv instead of the hardcoded one in the sdk. I will remove this, and it can become another discussion

should we give special treatment to service name? I feel like it will open a can of worms that which attributes should get dedicate builder method. I think we should keep the API lean for now.

Yes. As spec gives special treatment for service name, with a dedicated env variable just for that. No other Resource attribute has that treatment.

hdost

Thank you for the contribution 😊

hdost · 2024-11-26T03:28:41Z

opentelemetry-sdk/src/resource/builder.rs

+    }
+
+    /// Create ResourceBuilder with a default [Resource].
+    pub fn new_default() -> Self {


Suggested change

pub fn new_default() -> Self {

pub fn default() -> Self {

Probably should be done as an impl Default for ResourceBuilder to make it more generally compatible.

hdost · 2024-11-26T03:28:56Z

opentelemetry-sdk/src/resource/builder.rs

+
+impl ResourceBuilder {
+    /// Create ResourceBuilder with an empty [Resource].
+    pub fn new_empty() -> Self {


Suggested change

pub fn new_empty() -> Self {

pub fn new() -> Self {

This makes it easier to reason about. Since on Resource the new is the same as empty but it accepts key values. Since this is a builder most would assume it's empty.

lalitb · 2024-12-02T03:36:35Z

opentelemetry-sdk/src/resource/builder.rs

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


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

lalitb · 2024-12-02T03:47:07Z

opentelemetry-sdk/src/resource/builder.rs

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


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

lalitb · 2024-12-02T17:47:35Z

/// 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>, 
        
           {

cijothomas · 2024-12-03T18:33:41Z

opentelemetry-sdk/src/resource/builder.rs

+}
+
+impl Default for ResourceBuilder {
+    /// Create ResourceBuilder with [Resource::default()].


can we mention what the default would be here also? i.e it'll contain the telemetry,env etc.?

TommyCpp · 2024-12-03T21:31:35Z

opentelemetry-sdk/src/resource/builder.rs

+    }
+
+    /// Create a [Resource] with the options provided to the [ResourceBuilder].
+    pub fn build(self) -> Resource {


Can we implement Into<Resource> for ResourceBuilder to make it easier to use ResourceBuilder as Resource in parameters

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

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

pitoniak32 mentioned this pull request Nov 22, 2024

REQUEST: New membership for pitoniak32 open-telemetry/community#2452

Closed

6 tasks

pitoniak32 mentioned this pull request Nov 23, 2024

Feat/remove timeout from detectors #2332

Open

4 tasks

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

cijothomas reviewed Nov 24, 2024

View reviewed changes

opentelemetry-sdk/src/resource/mod.rs Outdated Show resolved Hide resolved

cijothomas reviewed Nov 24, 2024

View reviewed changes

opentelemetry-sdk/src/resource/builder.rs Outdated Show resolved Hide resolved

cijothomas reviewed Nov 24, 2024

View reviewed changes

stress/src/throughput.rs Outdated Show resolved Hide resolved

pitoniak32 force-pushed the feat/add-resource-builder branch 2 times, most recently from 5976ef0 to e501d4f Compare November 24, 2024 16:02

cijothomas reviewed Nov 24, 2024

View reviewed changes

hdost requested changes Nov 26, 2024

View reviewed changes

lalitb added this to the 0.27.1 milestone Nov 26, 2024

pitoniak32 added 8 commits December 1, 2024 15:26

feat: add ResourceBuilder for easily creating new Resources

6f2c4ce

chore: make ResourceBuilder pub

81e20ec

fix: use empty resource correctly

205735d

feat: add example of using ResourceBuilder

48b037e

fix: add missing timeout for detect

7545b70

feat: use new and default for ResourceBuilder

3397dd2

feat: add test for with_service_name

cae4219

chore: add entry for ResourceBuilder to CHANGELOG

155f875

pitoniak32 force-pushed the feat/add-resource-builder branch from 9fca93a to 155f875 Compare December 1, 2024 20:29

feat: remove with_service_name for further discussion

ee00eb5

lalitb reviewed Dec 2, 2024

View reviewed changes

Merge branch 'main' into feat/add-resource-builder

c00aff4

cijothomas removed this from the 0.27.1 milestone Dec 2, 2024

cijothomas reviewed Dec 3, 2024

View reviewed changes

TommyCpp reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/add resource builder #2322

Feat/add resource builder #2322

pitoniak32 commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

pitoniak32 commented Nov 22, 2024

utpilla commented Nov 23, 2024

pitoniak32 commented Nov 23, 2024

cijothomas Nov 24, 2024

pitoniak32 Nov 24, 2024

lalitb Dec 2, 2024

pitoniak32 commented Nov 24, 2024

cijothomas Nov 24, 2024

pitoniak32 Nov 24, 2024

pitoniak32 Dec 1, 2024

TommyCpp Dec 1, 2024 •

edited

Loading

pitoniak32 Dec 1, 2024

cijothomas Dec 3, 2024

hdost left a comment

hdost Nov 26, 2024

pitoniak32 Dec 1, 2024

hdost Nov 26, 2024

pitoniak32 Dec 1, 2024

lalitb Dec 2, 2024

cijothomas Dec 3, 2024

lalitb Dec 2, 2024

lalitb commented Dec 2, 2024

cijothomas Dec 3, 2024

TommyCpp Dec 3, 2024

Feat/add resource builder #2322

Are you sure you want to change the base?

Feat/add resource builder #2322

Conversation

pitoniak32 commented Nov 22, 2024 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

pitoniak32 commented Nov 22, 2024

utpilla commented Nov 23, 2024

pitoniak32 commented Nov 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitoniak32 commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyCpp Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitoniak32 commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

TommyCpp Dec 1, 2024 •

edited

Loading