gRPC refresh #2135

darach · 2022-12-15T14:07:24Z

Pull request

gRPC refresh.

Provoked by original work by @dak-x to add compression support to the otel connectors he discovered
that a lot more work would be required to upgrade tremor's gRPC connectivity across the board due to
changes in tonic ( gRPC ) and prost ( protobuf ) and changes to the protobuf specifications for OpenTelemetry
and the GCP connectors in tremor that leverage gRPC.

The relatively small and simple change. Was now no more a simple and quick undertaking.

This pull request captures the functional changes required to complete that work pending acceptance
tests against all the affected connectors as changes to GCP pulled in a new untested authentication
mechanism and library.

Once acceptance tests have been conducted the 😭 sob emoji may be removed and this draft promoted
to a full pull request.

Description

See above.

Checklist

The RFC, if required, has been submitted and approved
Any user-facing impact of the changes is reflected in docs.tremor.rs
The code is tested
Use of unsafe code is reasoned about in a comment
Update CHANGELOG.md appropriately, recording any changes, bug fixes, or other observable changes in behavior
The performance impact of the change is measured (see below)

Performance

No functional or critical path changes - changes to generated tonic/prost should be negligeable but we do not have CI for GCP based connectors at this time so have no automated guarantees or assurances in our CI/CD systems at this time.

darach · 2022-12-15T14:14:32Z

This draft assumes tremor-rs/tremor-otelapis#16 will be merged at some point - after we update github actions so that our CI has a working protoc 😬. Fun times.

Licenser

Woooh! Much grpc!

Licenser · 2022-12-15T14:16:12Z

CHANGELOG.md

@@ -10,15 +10,22 @@
 ## [0.13.0-rc.3]

 ### New features
-
+- Otel rebased to v19 specification version and deprecated v18 features removed. Breaking change.


this got to move up to "Unreleased"

Licenser · 2022-12-15T14:16:50Z

Cargo.toml

-prost = "0.9.0"
-prost-types = "0.9.0"
-tremor-otelapis = { version = "0.2.4" }
+tremor-otelapis = { path = "../tremor-otelapis" } # WAS ... , version = "0.2.5" }


I think this needs to change back to the version before we can merge it

Licenser · 2022-12-15T14:17:47Z

Cargo.toml

@@ -241,6 +245,10 @@ uuid = { version = "1.1", features = ["v4"] }

 # wal
 qwal = { git = "https://github.com/tremor-rs/qwal" }
+warp = "0.3.3"


probably should move up to the GCP section

Licenser · 2022-12-15T14:21:06Z

src/connectors/impls/gcl/writer/meta.rs

@@ -28,6 +28,7 @@ pub(crate) fn insert_id(meta: Option<&Value>) -> String {
    get_or_default(meta, "insert_id")
 }

+#[allow(clippy::cast_possible_wrap)] // casting seconds to u64 here is safe


<3 love the explaining comment! We should always do that!

Licenser · 2022-12-15T14:22:23Z

src/connectors/impls/gcl/writer/meta.rs

-            otherwise => Some(std::time::Duration::from_nanos(otherwise).into()),
+            otherwise => {
+                let mut duration = prost_types::Duration {
+                    seconds: otherwise as i64 / 1_000_000_000i64,


fringe case but should we move the cast to the outside to ensure we're not loosing a bit?

Suggested change

seconds: otherwise as i64 / 1_000_000_000i64,

seconds: (otherwise / 1_000_000_000u64) as i64,

Licenser · 2022-12-15T14:25:24Z

src/connectors/impls/gcl/writer/meta.rs

+    if let Some(split) = has_meta.get("split") {
+        return Some(LogSplit {
+            uid: split.get("uid").as_str().unwrap_or("").to_string(),
+            index: split.get("line").as_i32().unwrap_or(0),
+            total_splits: split.get("total_splits").as_i32().unwrap_or(0),
+        });
+    }
+
+    // Otherwise, None as mapping is optional
+    None


Option allows for ? which lets us unnest the code here, and get_<type> on value combines get and as :)

Suggested change

if let Some(split) = has_meta.get("split") {

return Some(LogSplit {

uid: split.get("uid").as_str().unwrap_or("").to_string(),

index: split.get("line").as_i32().unwrap_or(0),

total_splits: split.get("total_splits").as_i32().unwrap_or(0),

});

}

// Otherwise, None as mapping is optional

None

let split = has_meta.get("split")?;

Some(LogSplit {

uid: split.get_str("uid").unwrap_or("").to_string(),

index: split.get_i32("line").unwrap_or(0),

total_splits: split.get_i32("total_splits").unwrap_or(0),

})

Licenser · 2022-12-15T23:03:08Z

src/connectors/impls/otel/metrics.rs

-                value: maybe_from_value(data.get("value"))?,
+                value: match data.get("value") {
+                    Some(Value::Static(StaticNode::F64(v))) => Some(exemplar::Value::AsDouble(*v)),
+                    Some(Value::Static(StaticNode::I64(v))) => Some(exemplar::Value::AsInt(*v)),


It's important to use as_i64 instead of matching on the value here otherwise it'll not match for other number types (like u64 or later 128 bit types that would "fit") (Same below)

Licenser · 2022-12-19T12:58:49Z

Cargo.toml

@@ -208,13 +208,13 @@ sled = "0.34"

 # opentelemetry
 port_scanner = "0.1.5"
-tonic = { version = "0.6.1", default-features = false, features = [
+tonic = { version = "0.8.2", default-features = false, features = [


Suggested change

tonic = { version = "0.8.2", default-features = false, features = [

tonic = { version = "0.8.3", default-features = false, features = [

Signed-off-by: Darach Ennis <[email protected]>

Licenser · 2024-04-17T08:20:25Z

Does it make sense to keep this PR or a have things changed far enough after two years that it's worth closing this and taking a fresh look with the new connector design?

Licenser reviewed Dec 16, 2022

View reviewed changes

Licenser mentioned this pull request Dec 19, 2022

Bump tonic from 0.6.2 to 0.8.3 #2122

Closed

Licenser reviewed Dec 19, 2022

View reviewed changes

darach added 6 commits December 20, 2022 15:18

Patch in Daksh's otel compression contribution into current main

938720e

Signed-off-by: Darach Ennis <[email protected]>

Obey rust fmt

e690093

Signed-off-by: Darach Ennis <[email protected]>

gRPC refresh impacting Otel, GCP gRPC auth, GBQ write mode

2457507

Signed-off-by: Darach Ennis <[email protected]>

Reconcile differences between head and workstream and get tests passing

6696089

Signed-off-by: Darach Ennis <[email protected]>

Obey clippy

3a92a43

Signed-off-by: Darach Ennis <[email protected]>

Obey fmt

165a999

Signed-off-by: Darach Ennis <[email protected]>

darach force-pushed the daksh-otel-upgrades branch from d746943 to 165a999 Compare January 12, 2023 16:18

Remove extreneous await in G auth util code

d6f7e65

Signed-off-by: Darach Ennis <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC refresh #2135

gRPC refresh #2135

darach commented Dec 15, 2022

darach commented Dec 15, 2022

Licenser left a comment

Licenser Dec 15, 2022

Licenser Dec 15, 2022

Licenser Dec 15, 2022

Licenser Dec 15, 2022

Licenser Dec 15, 2022

Licenser Dec 15, 2022

Licenser Dec 15, 2022

Licenser Dec 19, 2022

Licenser commented Apr 17, 2024

	seconds: otherwise as i64 / 1_000_000_000i64,
	seconds: (otherwise / 1_000_000_000u64) as i64,

	tonic = { version = "0.8.2", default-features = false, features = [
	tonic = { version = "0.8.3", default-features = false, features = [

gRPC refresh #2135

Are you sure you want to change the base?

gRPC refresh #2135

Conversation

darach commented Dec 15, 2022

Pull request

Description

Related

Checklist

Performance

darach commented Dec 15, 2022

Licenser 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

Choose a reason for hiding this comment

Licenser commented Apr 17, 2024