Skip to content

Conversation

westarle
Copy link
Contributor

This structure will hold typed attributes for inclusion in spans and metrics for #3239. It extracts data from the request and response, and will have methods to create spans with consistent keys and values.

Copy link

codecov bot commented Sep 17, 2025

Codecov Report

❌ Patch coverage is 80.43478% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.17%. Comparing base (0431b38) to head (23cd643).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/gax-internal/src/observability.rs 80.43% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3352      +/-   ##
==========================================
- Coverage   96.29%   96.17%   -0.12%     
==========================================
  Files         114      115       +1     
  Lines        4530     4576      +46     
==========================================
+ Hits         4362     4401      +39     
- Misses        168      175       +7     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@westarle westarle force-pushed the impl/gax-internal-http-span-info branch from b6aa105 to 6d1a575 Compare September 17, 2025 19:49
@westarle
Copy link
Contributor Author

Codecov Report

❌ Patch coverage is 78.57143% with 9 lines in your changes missing coverage. Please review. ✅ Project coverage is 96.17%. Comparing base (0431b38) to head (6d1a575).

Files with missing lines Patch % Lines
src/gax-internal/src/observability.rs 78.57% 9 Missing ⚠️
Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3352      +/-   ##
==========================================
- Coverage   96.29%   96.17%   -0.12%     
==========================================
  Files         114      115       +1     
  Lines        4530     4572      +42     
==========================================
+ Hits         4362     4397      +35     
- Misses        168      175       +7     

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

🚀 New features to boost your workflow:

I would like to add tests for these in future integration tests (simulated network issues.) Unfortunately, there isn't a good way to produce reqwest::Error for testing. I noted this in #3239 "Simulate network/request errors"

@westarle westarle marked this pull request as ready for review September 17, 2025 20:52
@westarle westarle requested a review from a team as a code owner September 17, 2025 20:52
coryan
coryan previously approved these changes Sep 17, 2025
Copy link
Collaborator

@coryan coryan left a comment

Choose a reason for hiding this comment

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

I think we can make this structure a little less heavy, but we can also improve this after you merge it.

Comment on lines 18 to 20
/// Holds information extracted from HTTP requests and responses,
/// formatted to align with OpenTelemetry semantic conventions for tracing.
/// This struct is used to populate attributes on tracing spans.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: think of the documentation as "title + description":

Suggested change
/// Holds information extracted from HTTP requests and responses,
/// formatted to align with OpenTelemetry semantic conventions for tracing.
/// This struct is used to populate attributes on tracing spans.
/// Populate attributes of tracing spans for HTTP requests.
///
/// OpenTelemetry recommends a number of semantic conventions for
/// tracing HTTP requests. This type holds the information extracted
/// from HTTP requests and responses, formatted to align with these
/// OpenTelemetry semantic conventions.

If you want to be extra nice link the OTel semantic conventions place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, done!

Comment on lines 25 to 27
otel_kind: String, // "Client"
otel_name: String, // "{METHOD} {url.template}" or "{METHOD}"
otel_status: String, // "Unset", "Ok", "Error"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if these elements #[doc(hidden)] I think just using standard Rust documentation makes the code easier to read (and we could generate "private") documentation for ourselves:

Suggested change
otel_kind: String, // "Client"
otel_name: String, // "{METHOD} {url.template}" or "{METHOD}"
otel_status: String, // "Unset", "Ok", "Error"
/// The type of request, typically `Client`.
otel_kind: String,
/// The request name, either "{METHOD} {url.template}" or "{METHOD}"
otel_name: String,
/// The result of the request, one of: "Unset", "Ok", "Error"
otel_status: String,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all the doc strings.

Do I need to add any #[doc(hidden)] directive?

Copy link
Member

@dbolduc dbolduc Sep 18, 2025

Choose a reason for hiding this comment

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

No, you don't need to add any directives.

All of the modules are feature gated:

https://github.com/googleapis/google-cloud-rust/blob/main/src/gax-internal/src/lib.rs

And we tell docs.rs to build without any features enabled:

[package.metadata.docs.rs]
# Disable all features during document generation. All the types in this crate
# are implementation details, and subject to change without notice.
features = []

We could decide to enable features in a local cargo doc build for "private documentation". I guess.

Comment on lines 25 to 27
otel_kind: String, // "Client"
otel_name: String, // "{METHOD} {url.template}" or "{METHOD}"
otel_status: String, // "Unset", "Ok", "Error"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like it should be an enum OtelStatus or Option<OtelStatus> that has a name() -> &'static str accessor ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Open telemetry crate has enums like this, but I don't want to start mixing them into HTTP code yet (and this one is pretty simple).

otel_status: String, // "Unset", "Ok", "Error"

// OpenTelemetry Semantic Conventions
rpc_system: String, // "http"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why carry this in a field if it is a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, it seemed simpler to have fields for all recorded attributes; I'm still considering a macro that binds rpc_system to the string "rpc.system" once or potentially using this for RPCs -- it's convenient to carry them around as fields.

I think a future pass can lighten this up (so rpc_system is a ref) or remove it to a const.

// Custom GCP Attributes
gcp_client_service: Option<String>,
gcp_client_version: Option<String>,
gcp_client_repo: String, // "googleapis/google-cloud-rust"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto, isn't this always constant?

/// formatted to align with OpenTelemetry semantic conventions for tracing.
/// This struct is used to populate attributes on tracing spans.
#[derive(Debug, Clone)]
#[allow(dead_code)] // TODO(#3239): Remove once used in http.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

optional: I think you only need this when you do pub mod observability;

Comment on lines +166 to +174
if err.is_timeout() {
self.error_type = Some("TIMEOUT".to_string());
} else if err.is_connect() {
self.error_type = Some("CONNECTION_ERROR".to_string());
} else if err.is_request() {
self.error_type = Some("REQUEST_ERROR".to_string());
} else {
self.error_type = Some("UNKNOWN".to_string());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

optional:

Suggested change
if err.is_timeout() {
self.error_type = Some("TIMEOUT".to_string());
} else if err.is_connect() {
self.error_type = Some("CONNECTION_ERROR".to_string());
} else if err.is_request() {
self.error_type = Some("REQUEST_ERROR".to_string());
} else {
self.error_type = Some("UNKNOWN".to_string());
}
self.error_type = {
let name = if err.is_timeout() {
"TIMEOUT"
} else if err.is_connect() {
"CONNECTION_ERROR"
} else if err.is_request() {
"REQUEST_ERROR"
} else {
"UNKNOWN"
};
Some(name.to_string())
};

It may be possible to use .into() to simplify this further, or use a match, along the lines of match &err { e if e.is_timeout() => "TIMEOUT", ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3357 thanks!

@westarle westarle merged commit b128db3 into googleapis:main Sep 18, 2025
25 checks passed
@westarle westarle deleted the impl/gax-internal-http-span-info branch September 18, 2025 16:03
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.

3 participants