Skip to content

Conversation

@lalitb
Copy link
Member

@lalitb lalitb commented Jul 30, 2025

Changes

The User-Agent HTTP header should ideally identify the application making the request, not just the library.
This PR adds configurable User-Agent header support to the Geneva Uploader, allowing applications to identify themselves in HTTP requests to Geneva services.

So the approach with the PR is:

Configuration Resulting User-Agent
None RustGenevaClient/0.1
Some("MyApp/2.1.0") MyApp/2.1.0 (RustGenevaClient/0.1)

Also added support to provide the user-agent header in the Ingestion Service (along with Config Service)

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)

@lalitb lalitb requested a review from a team as a code owner July 30, 2025 23:01
@lalitb lalitb changed the title feat: [Geneva Exporter] Add configurable User-Agent Header Support feat: [Geneva Exporter] Add configurable User-Agent Header Jul 30, 2025
@utpilla
Copy link
Contributor

utpilla commented Aug 1, 2025

Changes

The User-Agent HTTP header should ideally identify the application making the request, not just the library.

If this is the only intention, then should we choose the User Agent ourselves instead of letting the user override it and potentially misuse or erroneously use it. We should be able to get hold of the executable name through std::env::args() or std::env::current_exe().

@codecov
Copy link

codecov bot commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 92.25352% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.1%. Comparing base (8b9ef08) to head (aa6fb5e).

Files with missing lines Patch % Lines
...eneva/geneva-uploader/src/ingestion_service/mod.rs 0.0% 4 Missing ⚠️
...r-geneva/geneva-uploader/src/config_service/mod.rs 83.3% 3 Missing ⚠️
stress/src/geneva_exporter.rs 0.0% 3 Missing ⚠️
...try-exporter-geneva/geneva-uploader-ffi/src/lib.rs 66.6% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #400     +/-   ##
=======================================
+ Coverage   53.6%   54.1%   +0.4%     
=======================================
  Files         71      72      +1     
  Lines      11632   11763    +131     
=======================================
+ Hits        6244    6364    +120     
- Misses      5388    5399     +11     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lalitb
Copy link
Member Author

lalitb commented Aug 1, 2025

If this is the only intention, then should we choose the User Agent ourselves instead of letting the user override it and potentially misuse or erroneously use it. We should be able to get hold of the executable name through std::env::args() or std::env::current_exe().

Intention is to provide the applications to provide the customization for the User-Agent- according to the format they want (which could be say MyApp/1.0

format!("{prefix} (GenevaUploader/0.1)")
};

HeaderValue::from_str(&user_agent).map_err(|e| {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this method does the ASCII characters validation check anyway so we could get avoid checking that ourselves.

for (i, ch) in prefix.char_indices() {
match ch {
// Control characters that would break HTTP headers
'\r' | '\n' | '\0' => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think had initially suggested checking for control characters in a comment, but I realized they would anyway be outside of the ASCII range so we would never allow control characters as long as we only accept ASCII.

And the ASCII check is already done by HeaderValue::from_str() method, so we can probably get rid of this whole method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

But I am good to remove the validation altogether, and let the reqwest crate handle it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems HeaderValue::from_str() allows non-ascii chars https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=1e264800416082c69cbb17c474caab02

This led to some new learnings. The documentation for HeaderValue::from_str() method says that it doesn't allow non-ASCII characters so I would have expected the test cases you shared to fail. I then found this is in the documentation for HeaderValue struct:

In practice, HTTP header field values are usually valid ASCII. However, the HTTP spec allows for a header value to contain opaque bytes as well. In this case, the header field value is not able to be represented as a string.

To handle this, the HeaderValue is useable as a type and can be compared with strings and implements Debug. A to_str fn is provided that returns an Err if the header value contains non visible ascii characters.

This means that we need to check if HeaderValue::from_str(str).is_ok() && HeaderValue::from_str(str).unwrap().to_str().is_ok() to rule out the non-ASCII inputs.

Check this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=9802b30e262c93e92e71fd92fcb543dd

pub async fn new(cfg: GenevaClientConfig) -> Result<Self, String> {
// Build config client config
// Validate user agent prefix once and build headers once for both services
// This avoids duplicate validation and header building in config and ingestion services
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a duplicate validation since build_geneva_headers method also calls validate_user_agent_prefix method.

@lalitb lalitb requested a review from utpilla November 3, 2025 23:54
@lalitb lalitb requested a review from Copilot November 4, 2025 01:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds configurable user-agent prefix support to the Geneva client, allowing applications to identify themselves in HTTP requests. The user agent format becomes "{prefix} (RustGenevaClient/0.1)" when a prefix is provided, or defaults to "RustGenevaClient/0.1" when None.

Key changes:

  • Added user_agent_prefix field to GenevaClientConfig with validation and documentation
  • Introduced common.rs module with header building utilities and comprehensive validation
  • Refactored header construction to centralize logic and enable header reuse across config and ingestion services

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
opentelemetry-exporter-geneva/geneva-uploader/src/common.rs New module containing header building utilities with validation for user-agent strings
opentelemetry-exporter-geneva/geneva-uploader/src/client.rs Added user_agent_prefix field to GenevaClientConfig with documentation and header building
opentelemetry-exporter-geneva/geneva-uploader/src/config_service/client.rs Refactored to use static headers from config, removed build_static_headers method
opentelemetry-exporter-geneva/geneva-uploader/src/ingestion_service/uploader.rs Added static_headers field to GenevaUploaderConfig and merged into client headers
opentelemetry-exporter-geneva/geneva-uploader/src/lib.rs Added common module to public exports
stress/src/geneva_exporter.rs Updated test configurations to include user_agent_prefix field
opentelemetry-exporter-geneva/opentelemetry-exporter-geneva/examples/*.rs Updated example configurations to include user_agent_prefix field
opentelemetry-exporter-geneva/geneva-uploader-ffi/src/lib.rs Added user_agent_prefix field with None default for FFI
opentelemetry-exporter-geneva/geneva-uploader/src/config_service/mod.rs Updated tests to build and pass static_headers
opentelemetry-exporter-geneva/geneva-uploader/src/ingestion_service/mod.rs Updated tests to build and pass static_headers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 35 to 44
/// User agent prefix for the application. Will be formatted as "<prefix> (GenevaUploader/0.1)".
/// If None, defaults to "GenevaUploader/0.1".
///
/// The prefix must contain only ASCII printable characters, be non-empty (after trimming),
/// and not exceed 200 characters in length.
///
/// Examples:
/// - None: "GenevaUploader/0.1"
/// - Some("MyApp/2.1.0"): "MyApp/2.1.0 (GenevaUploader/0.1)"
/// - Some("ProductionService-1.0"): "ProductionService-1.0 (GenevaUploader/0.1)"
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

The documentation states the format will use 'GenevaUploader/0.1' but the actual implementation in build_user_agent_header uses 'RustGenevaClient/0.1'. This inconsistency between documented and actual behavior should be corrected to match the implementation.

Suggested change
/// User agent prefix for the application. Will be formatted as "<prefix> (GenevaUploader/0.1)".
/// If None, defaults to "GenevaUploader/0.1".
///
/// The prefix must contain only ASCII printable characters, be non-empty (after trimming),
/// and not exceed 200 characters in length.
///
/// Examples:
/// - None: "GenevaUploader/0.1"
/// - Some("MyApp/2.1.0"): "MyApp/2.1.0 (GenevaUploader/0.1)"
/// - Some("ProductionService-1.0"): "ProductionService-1.0 (GenevaUploader/0.1)"
/// User agent prefix for the application. Will be formatted as "<prefix> (RustGenevaClient/0.1)".
/// If None, defaults to "RustGenevaClient/0.1".
///
/// The prefix must contain only ASCII printable characters, be non-empty (after trimming),
/// and not exceed 200 characters in length.
///
/// Examples:
/// - None: "RustGenevaClient/0.1"
/// - Some("MyApp/2.1.0"): "MyApp/2.1.0 (RustGenevaClient/0.1)"
/// - Some("ProductionService-1.0"): "ProductionService-1.0 (RustGenevaClient/0.1)"

Copilot uses AI. Check for mistakes.
Comment on lines 35 to 44
/// User agent prefix for the application. Will be formatted as "<prefix> (GenevaUploader/0.1)".
/// If None, defaults to "GenevaUploader/0.1".
///
/// The prefix must contain only ASCII printable characters, be non-empty (after trimming),
/// and not exceed 200 characters in length.
///
/// Examples:
/// - None: "GenevaUploader/0.1"
/// - Some("MyApp/2.1.0"): "MyApp/2.1.0 (GenevaUploader/0.1)"
/// - Some("ProductionService-1.0"): "ProductionService-1.0 (GenevaUploader/0.1)"
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

The examples show 'GenevaUploader/0.1' but the actual format produced by the implementation is 'RustGenevaClient/0.1'. All examples should be updated to reflect the actual implementation.

Suggested change
/// User agent prefix for the application. Will be formatted as "<prefix> (GenevaUploader/0.1)".
/// If None, defaults to "GenevaUploader/0.1".
///
/// The prefix must contain only ASCII printable characters, be non-empty (after trimming),
/// and not exceed 200 characters in length.
///
/// Examples:
/// - None: "GenevaUploader/0.1"
/// - Some("MyApp/2.1.0"): "MyApp/2.1.0 (GenevaUploader/0.1)"
/// - Some("ProductionService-1.0"): "ProductionService-1.0 (GenevaUploader/0.1)"
/// User agent prefix for the application. Will be formatted as "<prefix> (RustGenevaClient/0.1)".
/// If None, defaults to "RustGenevaClient/0.1".
///
/// The prefix must contain only ASCII printable characters, be non-empty (after trimming),
/// and not exceed 200 characters in length.
///
/// Examples:
/// - None: "RustGenevaClient/0.1"
/// - Some("MyApp/2.1.0"): "MyApp/2.1.0 (RustGenevaClient/0.1)"
/// - Some("ProductionService-1.0"): "ProductionService-1.0 (RustGenevaClient/0.1)"

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants