Add GetValue and GetValues Support #75

erikbosch · 2024-09-30T11:19:04Z

No description provided.

codecov · 2024-09-30T11:24:01Z

Codecov Report

Attention: Patch coverage is 93.23607% with 51 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature/databroker-api-v2@4eee61c). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
databroker/src/grpc/kuksa_val_v2/val.rs	93.20%	51 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature/databroker-api-v2      #75   +/-   ##
============================================================
  Coverage                             ?   53.18%           
============================================================
  Files                                ?       33           
  Lines                                ?    13675           
  Branches                             ?        0           
============================================================
  Hits                                 ?     7273           
  Misses                               ?     6402           
  Partials                             ?        0

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

erikbosch · 2024-09-30T11:26:57Z

databroker/src/grpc/kuksa_val_v2/conversions.rs

@@ -45,7 +45,10 @@ impl From<&proto::Datapoint> for broker::Datapoint {
 impl From<broker::Datapoint> for Option<proto::Datapoint> {
    fn from(from: broker::Datapoint) -> Self {
        match from.value {
-            broker::DataValue::NotAvailable => None,
+            broker::DataValue::NotAvailable => Some(proto::Datapoint {


This one is worth discussing. Previously (for V2) we just ignored "NotAvailable", i.e. it was never returned in subscribe. But at lest for GetValue we want to return it, so I move the filtering higher up.

But how do we want it to work for Subscribe? If we intend to support actively removing a DataPoint by providing a DataPoint with value Null, then maybe we want the subscribe to also have support for returning Null.

And if so, would it not make sense to also return Null in the first request when subscription is started?

(Currently PR does not change behavior for Subscribe)

Discussed among committers - it seems we want None to be returned also for subscribe. I will check how it affects test case and update the PR

Was not part of the discussion but I think there you can see how it returns None if DataValue::NotAvailable ->

kuksa-databroker/databroker/src/grpc/kuksa_val_v2/val.rs

Line 669 in 580f25b

broker::DataValue::NotAvailable => None,

It returns None there, but as it returns None there it means that the value will be discarded (and not sent to client) at https://github.com/eclipse-kuksa/kuksa-databroker/blob/feature/databroker-api-v2/databroker/src/grpc/kuksa_val_v2/val.rs#L638

Thats why we need to create a Datapoint with value None here.

Now returns value None when subscription started if there are no value present

databroker/src/grpc/kuksa_val_v2/val.rs

lukasmittag · 2024-10-01T07:58:46Z

proto/kuksa/val/v2/val.proto

  //
  // Returns (GRPC error code):
  //   NOT_FOUND if any of the requested signals doesn't exist.
  //   PERMISSION_DENIED if access is denied for any of the requested signals.
+  //
  rpc GetValues(GetValuesRequest) returns (GetValuesResponse);


General question: Do we need to split this as well by GetValuesId' and GetValues`?

The message SignalID supports both, so you can use whatever you like with the same RPC call.

(No tests yet)

proto/kuksa/val/v2/val.proto

lukasmittag · 2024-10-04T07:52:06Z

databroker/src/grpc/kuksa_val_v2/conversions.rs

-            broker::DataValue::NotAvailable => None,
+            broker::DataValue::NotAvailable => Some(proto::Datapoint {
+                value: None,
+                timestamp: None,


why should it not have a timestamp?

Fo "normal" DataPoints that has a value the timestamp represents the time the databroker registered that value (doesn't it?). In this case Databroker "deep level magic" has no value and provide no timestamp (I think). We could opt for adding current time if we like to show that it was None at Now, but "Now" is the only time we have.

But a good point to dicuss, do we want a timestamp when return Value==None and if so what Timestamp. (Theoretically we could request Databroker to keep track of when a Value change from something to None, but I do not think it do it at the moment)

It actually seems to return something, at least in some cases, so I will see if I can use it

So it returns registration time for the signal for "virgin" signals, we could use that.

yes, that's what I thought of and IF the signal provider would go away you would have a timestamp as well.

So it returns registration time for the signal for "virgin" signals, we could use that.

Setting the value to NotAvailable behaves just like every other value, at least when it comes to sdv.databroker.v1 (and hence it's supported by the core of databroker).

Setting a value to 20.0 will record the timestamp of when that happened.

Setting a value to NotAvailable will record the timestamp of when that happened.

lukasmittag · 2024-10-04T07:53:07Z

databroker/src/grpc/kuksa_val_v2/val.rs

+                return Err(tonic::Status::permission_denied("Permission denied"))
+            }
+            Err(ReadError::PermissionExpired) => {
+                return Err(tonic::Status::unauthenticated("Unauthenticated"))


Wouldn't it be beneficial if we tell them in the message "Unauthenticated because your token expired"?

Err(ReadError::PermissionExpired) => { return Err(tonic::Status::unauthenticated(format!( "Permission expired (id: {})", signal_id )))

like here in line 120 ff.

Should the Unauthenticated status also be added to the GetValue service documentation in the proto file?

Makes sens to say that auth expired.

There are theoretically a lot of GRPC error codes (see for example https://grpc.github.io/grpc/cpp/md_doc_statuscodes.html) that we cannot control but the client still can receive. But it might be a good policy to mention all that we explicitly return in our implementation

databroker/src/grpc/kuksa_val_v2/val.rs

rafaeling

Looks good to me, thanks for the documenting the proto files and thanks a lot for adding those really nice unit tests :)

rafaeling · 2024-10-04T08:04:56Z

databroker/src/grpc/kuksa_val_v2/val.rs

+                return Err(tonic::Status::permission_denied("Permission denied"))
+            }
+            Err(ReadError::PermissionExpired) => {
+                return Err(tonic::Status::unauthenticated("Unauthenticated"))


Should the Unauthenticated status also be added to the GetValue service documentation in the proto file?

databroker/src/grpc/kuksa_val_v2/val.rs

proto/kuksa/val/v2/val.proto

rafaeling · 2024-10-04T09:04:09Z

databroker/src/grpc/kuksa_val_v2/val.rs

+            .first_exist()
+            .request_first()
+            .send_auth()
+            .auth_first()


If auth_first or auth_second are set to True should not be send_auth True by default?

Well, at least there is no reason to have auth_first as True if you do not intend to send it. There are generally four combos I check:

We do not send credentials at all

We send credentials but they are empty

We send credentials for signal 1

We send credentials for signal 2

We send credentials for signal 1 and 2

I can change so that send_auth becomes optional if you call one of the other

Maybe then leave it as it is, is not a hurdle to call send_auth if auth_first or auth_second are set to True

You changed it already, thanks

wba2hi

Tested using the following commands:

ghz --insecure --skipFirst 100 --total 100100 --proto ./proto/kuksa/val/v2/val.proto --call kuksa.val.v2.VAL.GetValue -d '{"signal_id": {"path": "Vehicle.Speed"}}' 0.0.0.0:55556

Summary:
  Count:	100000
  Total:	8.96 s
  Slowest:	17.98 ms
  Fastest:	0.18 ms
  Average:	4.20 ms
  Requests/sec:	11159.41

Response time histogram:
  0.177  [1]     |
  1.958  [4235]  |∎∎∎
  3.738  [25572] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  5.519  [57218] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  7.299  [11867] |∎∎∎∎∎∎∎∎
  9.080  [922]   |∎
  10.860 [93]    |
  12.641 [52]    |
  14.421 [37]    |
  16.202 [1]     |
  17.982 [2]     |

Latency distribution:
  10 % in 2.50 ms
  25 % in 3.53 ms
  50 % in 4.25 ms
  75 % in 4.72 ms
  90 % in 5.84 ms
  95 % in 6.51 ms
  99 % in 7.36 ms

Status code distribution:
  [OK]   100000 responses

ghz --insecure --skipFirst 100 --total 100100 --proto ./proto/kuksa/val/v2/val.proto --call kuksa.val.v2.VAL.GetValue -d '{"signal_id": {"id": 100}}' 0.0.0.0:55556

Summary:
  Count:	100000
  Total:	8.68 s
  Slowest:	12.33 ms
  Fastest:	0.15 ms
  Average:	4.07 ms
  Requests/sec:	11525.19

Response time histogram:
  0.151  [1]     |
  1.368  [1093]  |∎
  2.586  [10441] |∎∎∎∎∎∎∎∎
  3.803  [23750] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  5.021  [49185] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  6.238  [10316] |∎∎∎∎∎∎∎∎
  7.456  [4902]  |∎∎∎∎
  8.673  [203]   |
  9.891  [52]    |
  11.108 [23]    |
  12.326 [34]    |

Latency distribution:
  10 % in 2.46 ms
  25 % in 3.42 ms
  50 % in 4.14 ms
  75 % in 4.59 ms
  90 % in 5.58 ms
  95 % in 6.27 ms
  99 % in 7.02 ms

Status code distribution:
  [OK]   100000 responses

ghz --insecure --skipFirst 100 --total 100100 --proto ./proto/kuksa/val/v2/val.proto --call kuksa.val.v2.VAL.GetValues -d '{"signal_ids": [{"path": "Vehicle.Speed"},{"path": "Vehicle.ADAS.ABS.IsEnabled"},{"id": 100}]}' 0.0.0.0:55556

Summary:
  Count:	100000
  Total:	8.90 s
  Slowest:	19.57 ms
  Fastest:	0.19 ms
  Average:	4.18 ms
  Requests/sec:	11241.01

Response time histogram:
  0.194  [1]     |
  2.131  [5680]  |∎∎∎∎
  4.068  [34113] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  6.005  [52709] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  7.942  [7293]  |∎∎∎∎∎∎
  9.880  [111]   |
  11.817 [43]    |
  13.754 [0]     |
  15.691 [0]     |
  17.629 [4]     |
  19.566 [46]    |

Latency distribution:
  10 % in 2.54 ms
  25 % in 3.58 ms
  50 % in 4.25 ms
  75 % in 4.69 ms
  90 % in 5.68 ms
  95 % in 6.37 ms
  99 % in 7.14 ms

Status code distribution:
  [OK]   100000 responses

Also tested the error case, where invalid VSS Paths or invalid ids are used.
Performance-wise it looks comparable to kuksa.val.v1.Get:

ghz --insecure --skipFirst 100 --total 100100 --proto ./proto/kuksa/val/v1/val.proto --call kuksa.val.v1.VAL.Get -d '{"entries": {"path": "Vehicle.Speed", "view": 1, "fields": 2}}' 0.0.0.0:55556

Summary:
  Count:	100000
  Total:	9.97 s
  Slowest:	16.46 ms
  Fastest:	0.52 ms
  Average:	4.72 ms
  Requests/sec:	10030.76

Response time histogram:
  0.519  [1]     |
  2.113  [1758]  |∎
  3.706  [13936] |∎∎∎∎∎∎∎∎∎
  5.300  [60628] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  6.894  [18435] |∎∎∎∎∎∎∎∎∎∎∎∎
  8.488  [4938]  |∎∎∎
  10.081 [242]   |
  11.675 [51]    |
  13.269 [10]    |
  14.863 [0]     |
  16.457 [1]     |

Latency distribution:
  10 % in 3.19 ms
  25 % in 4.15 ms
  50 % in 4.72 ms
  75 % in 5.26 ms
  90 % in 6.22 ms
  95 % in 6.93 ms
  99 % in 7.86 ms

Status code distribution:
  [OK]   100000 responses

Behavior is correct, therefore LGTM

erikbosch · 2024-10-07T12:09:11Z

Thanks for testing @wba2hi

* Add GetValue and GetValues Support

Add GetValue Support

580f25b

erikbosch mentioned this pull request Sep 30, 2024

Adding GetValue support boschglobal/kuksa-databroker#4

Closed

erikbosch commented Sep 30, 2024

View reviewed changes

erikbosch marked this pull request as ready for review September 30, 2024 11:55

erikbosch requested review from rafaeling, argerus and lukasmittag September 30, 2024 11:55

Refactored Subscription

098ccd3

Now returns value None when subscription started if there are no value present

erikbosch force-pushed the erik_get_value branch from 03493b5 to 098ccd3 Compare October 1, 2024 07:32

lukasmittag reviewed Oct 1, 2024

View reviewed changes

databroker/src/grpc/kuksa_val_v2/val.rs Outdated Show resolved Hide resolved

lukasmittag reviewed Oct 1, 2024

View reviewed changes

Add initial implementation of GetValues/ListValues

caa02bf

(No tests yet)

erikbosch commented Oct 1, 2024

View reviewed changes

proto/kuksa/val/v2/val.proto Show resolved Hide resolved

erikbosch marked this pull request as draft October 2, 2024 07:25

Second step of GetValues

d54c4fb

erikbosch force-pushed the erik_get_value branch from a1f12c0 to d54c4fb Compare October 2, 2024 14:36

erikbosch changed the title ~~Add GetValue Support~~ Add GetValue and GetValues Support Oct 2, 2024

erikbosch marked this pull request as ready for review October 2, 2024 14:39

erikbosch requested a review from lukasmittag October 2, 2024 14:39

lukasmittag reviewed Oct 4, 2024

View reviewed changes

rafaeling reviewed Oct 4, 2024

View reviewed changes

Update after review comments

b15123f

erikbosch requested review from rafaeling and lukasmittag October 4, 2024 11:38

wba2hi approved these changes Oct 7, 2024

View reviewed changes

erikbosch merged commit 435e88f into eclipse-kuksa:feature/databroker-api-v2 Oct 7, 2024
23 checks passed

erikbosch added a commit that referenced this pull request Oct 10, 2024

Add GetValue and GetValues Support (#75)

51d999c

* Add GetValue and GetValues Support

erikbosch added a commit that referenced this pull request Oct 10, 2024

Add GetValue and GetValues Support (#75)

4dce6f3

* Add GetValue and GetValues Support

erikbosch added a commit that referenced this pull request Oct 15, 2024

Add GetValue and GetValues Support (#75)

355163e

* Add GetValue and GetValues Support

erikbosch added a commit that referenced this pull request Oct 22, 2024

Add GetValue and GetValues Support (#75)

2897171

* Add GetValue and GetValues Support

argerus pushed a commit that referenced this pull request Nov 5, 2024

Add GetValue and GetValues Support (#75)

8c9aba2

* Add GetValue and GetValues Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetValue and GetValues Support #75

Add GetValue and GetValues Support #75

erikbosch commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

erikbosch Sep 30, 2024

erikbosch Sep 30, 2024

rafaeling Sep 30, 2024

erikbosch Oct 1, 2024

lukasmittag Oct 1, 2024

erikbosch Oct 1, 2024

lukasmittag Oct 4, 2024

erikbosch Oct 4, 2024

erikbosch Oct 4, 2024

erikbosch Oct 4, 2024

lukasmittag Oct 4, 2024

argerus Oct 4, 2024 •

edited

Loading

lukasmittag Oct 4, 2024

rafaeling Oct 4, 2024

erikbosch Oct 4, 2024

rafaeling left a comment

rafaeling Oct 4, 2024

rafaeling Oct 4, 2024

erikbosch Oct 4, 2024

rafaeling Oct 7, 2024

rafaeling Oct 7, 2024

wba2hi left a comment •

edited

Loading

erikbosch commented Oct 7, 2024

Add GetValue and GetValues Support #75

Add GetValue and GetValues Support #75

Conversation

erikbosch commented Sep 30, 2024 • edited Loading

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

argerus Oct 4, 2024 • 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

rafaeling 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

wba2hi left a comment • edited Loading

Choose a reason for hiding this comment

erikbosch commented Oct 7, 2024

erikbosch commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

argerus Oct 4, 2024 •

edited

Loading

wba2hi left a comment •

edited

Loading