otel feature with tracing functionality only #119

LikhithST · 2024-12-12T06:18:58Z

as suggested in pull request #97 this PR contains only the tracing functionality without the context transfer between clients and the databroker

SebastianSchildt · 2024-12-12T13:12:11Z

@lukasmittag or @argerus please check if this is acceptable/non-invasive enough and if so approve. Would be nice to have basic OTEL support (we can still think about how to do the context transfer "nicer" in a follow up PR)

codecov · 2024-12-12T13:15:22Z

Codecov Report

Attention: Patch coverage is 54.94505% with 41 lines in your changes missing coverage. Please review.

Project coverage is 59.55%. Comparing base (2159d18) to head (74bab0d).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
databroker/src/lib.rs	0.00%	23 Missing ⚠️
databroker/src/open_telemetry.rs	0.00%	15 Missing ⚠️
databroker/src/grpc/kuksa_val_v1/val.rs	81.81%	2 Missing ⚠️
databroker/src/grpc/kuksa_val_v1/conversions.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   59.49%   59.55%   +0.06%     
==========================================
  Files          33       34       +1     
  Lines       16070    15168     -902     
==========================================
- Hits         9561     9034     -527     
+ Misses       6509     6134     -375

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

SebastianSchildt · 2024-12-29T10:57:46Z

@LikhithST some minor mechanical linting issues: cargo fmt or clippy not happy, missing license identifier in otel.rs and some newline issue that can be fixed with the setting up pre-commit locally as described here https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/CONTRIBUTING.md#pre-commit-set-up

LikhithST · 2025-01-09T09:47:58Z

Hi @SebastianSchildt, I have added the license and ran the pre-commit hooks. I ran cargo clippy and cargo fmt to fix linting issues. The cargo fmt command modified a few files.

SebastianSchildt · 2025-01-09T10:06:58Z

Hi @LikhithST Thank you! From my side looks good. I will keep it open until next week when @lukasmittag is back to check whether has some additional comments. If not I will merge

SebastianSchildt

Looks reasonable to me 👍

rafaeling

Thanks for adding this great feature to databroker.

One important thing: at some point otel will also be added to kuksa.val.v2. Could you please review and add the kuksa_val_v1 prefix to otel name variables under kuksa_val_v1 folder, and ensure also that the correct file name prefix is present in all the name variables?

Pattern example:
name="class_name_+method_name"

Pattern example for files under kuksa_val_v1
name="api_+class_name_+method_name"

Thanks again.

rafaeling · 2025-01-13T09:10:25Z

databroker/src/broker.rs

@@ -706,6 +711,7 @@ impl Entry {
        }
    }

+    #[cfg_attr(feature="otel", tracing::instrument(name="apply_lag_after_execute", skip(self), fields(timestamp=chrono::Utc::now().to_string())))]


Add broker_ prefix

Thanks for the feedback,

Do I have to follow this pattern name="class_name_+method_name" strictly, because previously I have used this pattern only if more than one class have same method name.

Example:
for the method validate of class Entry, name="broker_validate", I have followed name=file_name+method_name.

Whereas for method notify of class ChangeSubscription, name=broker_change_subscription_notify here I have followed name=file_name+class_name+method_name

similarly for method notify of class QuerySubscription, name="broker_query_subscription_notify"

The best approach from my point of view is to use the pattern name="class_name_+method_name".

That way we can keep it short and concise so we can identify to which class belongs the method.

Also, find it too long the pattern:
name=file_name+class_name+method_name

rafaeling · 2025-01-13T09:19:29Z

databroker/src/broker.rs

    pub fn add_change_subscription(&mut self, subscription: ChangeSubscription) {
        self.change_subscriptions.push(subscription)
    }

+    #[cfg_attr(
+        feature = "otel",
+        tracing::instrument(name = "broker_Subscriptions_notify", skip(self, changed, db))


Please use lower case pattern and remove blank space between name and it value.

rafaeling · 2025-01-13T09:19:50Z

databroker/src/broker.rs

@@ -749,10 +755,15 @@ impl Subscriptions {
        self.query_subscriptions.push(subscription)
    }

+    #[cfg_attr(feature="otel", tracing::instrument(name = "broker_add_change_subscription",skip(self, subscription), fields(timestamp=chrono::Utc::now().to_string())))]


Remove blank space between name and its value.

rafaeling · 2025-01-13T09:21:09Z

databroker/src/broker.rs

@@ -835,6 +847,10 @@ impl Subscriptions {
 }

 impl ChangeSubscription {
+    #[cfg_attr(
+        feature = "otel",
+        tracing::instrument(name = "broker_ChangeSubscription_notify", skip(self, changed, db))


Please use lower case pattern and remove blank space between name and its value.

rafaeling · 2025-01-13T09:45:36Z

databroker/src/broker.rs

@@ -706,6 +711,7 @@ impl Entry {
        }
    }

+    #[cfg_attr(feature="otel", tracing::instrument(name="apply_lag_after_execute", skip(self), fields(timestamp=chrono::Utc::now().to_string())))]
    pub fn apply_lag_after_execute(&mut self) {
        self.lag_datapoint = self.datapoint.clone();
    }


Please also add otel to the apply method https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/databroker/src/broker.rs#L713
It would be also revelant to know how long does it take to initialize the hashset in the heap.

rafaeling · 2025-01-13T09:49:29Z

databroker/src/grpc/kuksa_val_v1/val.rs

@@ -256,11 +256,20 @@ impl proto::val_server::Val for broker::DataBroker {
        }
    }

+    #[cfg_attr(feature="otel",tracing::instrument(name="val_set",skip(self, request), fields(trace_id, timestamp= chrono::Utc::now().to_string())))]


Can you add kuksa_val_v1 as prefix to the name so it is clear to which val file belongs? Kuksa.val.v2 also contain val.rs file

rafaeling · 2025-01-13T09:49:50Z

databroker/src/grpc/kuksa_val_v1/val.rs

@@ -472,6 +481,7 @@ impl proto::val_server::Val for broker::DataBroker {
        >,
    >;

+    #[cfg_attr(feature="otel", tracing::instrument(name="subscribe", skip(self, request), fields(trace_id, timestamp=chrono::Utc::now().to_string())))]


Where do you get trace_id here from?
Please also add kuksa_val_v1_val_ as prefix.

trace_id is not being used here, I will remove it.

rafaeling · 2025-01-13T09:52:10Z

databroker/src/grpc/kuksa_val_v1/val.rs

+            Some(entry) => match &entry.metadata {
+                Some(metadata) => match &metadata.description {
+                    Some(description) => {
+                        trace_id = String::from(description);


Why is it used here description as trace_id instead of path?

Apologies for using description to handle trace_id, could comment in proto file be used for this purpose?

trace_id is introduced as an effort to identify the request with its corresponding otel tracing result. could I use comment that is a member of message Metadata.

What about cloning the path from the entry request?

adding otel tracing

a4d4955

LikhithST mentioned this pull request Dec 12, 2024

"Add 'otel' feature for OpenTelemetry support #97

Open

SebastianSchildt requested review from lukasmittag and argerus December 12, 2024 13:12

LikhithST added 3 commits January 9, 2025 10:05

adding license

431f042

newline fix

da63573

formatting changes

74bab0d

SebastianSchildt approved these changes Jan 9, 2025

View reviewed changes

rafaeling reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

otel feature with tracing functionality only #119

otel feature with tracing functionality only #119

LikhithST commented Dec 12, 2024

SebastianSchildt commented Dec 12, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

SebastianSchildt commented Dec 29, 2024

LikhithST commented Jan 9, 2025 •

edited

Loading

SebastianSchildt commented Jan 9, 2025

SebastianSchildt left a comment

rafaeling left a comment •

edited

Loading

rafaeling Jan 13, 2025

LikhithST Jan 14, 2025 •

edited

Loading

rafaeling Jan 16, 2025

rafaeling Jan 13, 2025

rafaeling Jan 13, 2025

rafaeling Jan 13, 2025

rafaeling Jan 13, 2025

rafaeling Jan 13, 2025

rafaeling Jan 13, 2025

LikhithST Jan 14, 2025

rafaeling Jan 13, 2025 •

edited

Loading

LikhithST Jan 14, 2025 •

edited

Loading

rafaeling Jan 16, 2025

otel feature with tracing functionality only #119

Are you sure you want to change the base?

otel feature with tracing functionality only #119

Conversation

LikhithST commented Dec 12, 2024

SebastianSchildt commented Dec 12, 2024

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

SebastianSchildt commented Dec 29, 2024

LikhithST commented Jan 9, 2025 • edited Loading

SebastianSchildt commented Jan 9, 2025

SebastianSchildt left a comment

Choose a reason for hiding this comment

rafaeling left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LikhithST Jan 14, 2025 • edited Loading

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

Choose a reason for hiding this comment

rafaeling Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

LikhithST Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2024 •

edited

Loading

LikhithST commented Jan 9, 2025 •

edited

Loading

rafaeling left a comment •

edited

Loading

LikhithST Jan 14, 2025 •

edited

Loading

rafaeling Jan 13, 2025 •

edited

Loading

LikhithST Jan 14, 2025 •

edited

Loading