Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: user project telemetry #1961

Merged
merged 15 commits into from
Jan 20, 2025
Merged

Conversation

the-wondersmith
Copy link
Contributor

@the-wondersmith the-wondersmith commented Jan 15, 2025

Description of change

PR expands on the existing tracing setup/configuration feature in shuttle-runtime, adding an enhanced subscriber that includes an OpenTelemetry layer and enables exporting all telemetry data (traces, metrics, and logs) generated by a Shuttle project to any compatible OTLP collector.

The new behavior is gated behind a new setup-telemetry feature flag.

Important

PR changes the default RUST_LOG configuration from info to info,{crate_name}=debug

@the-wondersmith the-wondersmith force-pushed the feat/user-project-telemetry branch from 058003a to 34ef4b5 Compare January 15, 2025 19:54
Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Thanks for the typo fixes :)
Remember to keep changes to legacy code few.

runtime/Cargo.toml Show resolved Hide resolved
common/src/resource.rs Outdated Show resolved Hide resolved
runtime/src/start.rs Show resolved Hide resolved
common/src/resource.rs Outdated Show resolved Hide resolved
runtime/src/start.rs Outdated Show resolved Hide resolved
@the-wondersmith the-wondersmith marked this pull request as ready for review January 16, 2025 13:40
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces OpenTelemetry support to the Shuttle runtime with a focus on telemetry data export. Here are the key points to consider:

  • Added new trace.rs with hardcoded configuration values for metrics interval and trace sampling rate that should be made configurable
  • Potential performance concerns with string allocations and cloning in hot paths in the LogCourier implementation
  • Several unwrap() calls in trace.rs that could fail in production and should be handled more gracefully

The changes look solid overall but would benefit from making the configuration more flexible and improving error handling robustness.

8 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

runtime/Cargo.toml Show resolved Hide resolved
codegen/src/shuttle_main.rs Outdated Show resolved Hide resolved
runtime/src/start.rs Outdated Show resolved Hide resolved
runtime/src/start.rs Outdated Show resolved Hide resolved
runtime/src/trace.rs Outdated Show resolved Hide resolved
@jonaro00 jonaro00 changed the title Feat/user project telemetry feat: user project telemetry Jan 16, 2025
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This PR continues the implementation of user project telemetry in Shuttle. Here are the key changes in this update:

  • Removes ProjectTelemetryConfig variant from ResourceTypeBeta enum in common/src/resource.rs, indicating a shift from resource-based to feature flag-based telemetry configuration
  • Simplifies database interactions by removing SQLx trait implementations for telemetry configuration
  • Moves telemetry setup behind the new setup-telemetry feature flag for better control and flexibility

The changes align with best practices by:

  • Keeping modifications to legacy code minimal
  • Using feature flags for controlled rollout
  • Simplifying the data model by removing database dependencies

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the provided context, I'll summarize the latest changes in this PR:

Added string conversion and display functionality to ResourceTypeBeta enum to support OpenTelemetry integration, with minimal changes to legacy code.

  • Added AsRefStr, EnumString, and Display derive macros to ResourceTypeBeta enum in common/src/resource.rs for improved string handling
  • Reordered existing derive macros for better code organization and readability
  • Maintained existing test coverage to ensure functionality remains intact

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the latest changes and previous reviews, here's my analysis of the recent updates:

The PR adds OpenTelemetry integration to the Shuttle runtime's tracing system. Here are the key new points to consider:

  • Added project name and version parameters to runtime start function in shuttle_main.rs for telemetry context enrichment
  • Introduced new setup-telemetry feature flag in runtime/Cargo.toml that's separate from existing setup-tracing feature
  • Implemented graceful shutdown handling in ProviderGuard to ensure proper cleanup of telemetry resources

The changes maintain good separation of concerns while adding telemetry capabilities through feature flags, though some configuration values remain hardcoded.

8 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

runtime/Cargo.toml Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the latest changes and previous reviews, here's my analysis of the recent updates:

Adds OpenTelemetry integration to the Shuttle runtime's tracing system with improved error handling and resource cleanup.

  • Improved error handling in ProviderGuard::drop() with proper error logging for tracer, meter and logger shutdown failures
  • Added SpanFieldVisitor trait and implementations for efficient field value handling in LogCourier
  • Introduced EventFieldValues struct with field remapping support for standardized log attribute names

The changes show good attention to error handling and performance optimization while maintaining clean code organization.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my analysis of the latest updates:

The PR continues to refine OpenTelemetry integration in the Shuttle runtime with dependency cleanup and feature flag improvements.

  • Removed redundant reqwest-rustls feature from opentelemetry-otlp in runtime/Cargo.toml since rustls support is included by default
  • Removed unused dependencies tokio-util and uuid from runtime/Cargo.toml for better maintenance
  • Introduced potential version mismatch between tracing-opentelemetry v0.28.0 and other OpenTelemetry crates at v0.27 that should be addressed

3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

runtime/Cargo.toml Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the latest changes and previous reviews, I'll provide a concise summary of the most recent updates:

Optimized string handling in resource type conversions with minimal impact to existing functionality.

  • Changed to_string() to as_ref() in common/src/resource.rs for more efficient string conversion of ResourceTypeBeta
  • Maintains existing functionality while reducing unnecessary string allocations
  • Aligns with previous feedback to keep legacy code changes minimal

The change is focused and efficient, improving performance without disrupting the existing resource handling system.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my analysis of the latest updates to the OpenTelemetry integration:

  • Added new setup-telemetry feature in runtime/Cargo.toml that's independent of existing setup-tracing feature, providing better control over telemetry capabilities
  • Changed default RUST_LOG configuration from info to info,{crate_name}=debug for more detailed logging
  • Version mismatch between tracing-opentelemetry v0.28.0 and other OpenTelemetry crates at v0.27 needs to be resolved

The changes maintain good separation of concerns while enhancing observability, though version compatibility should be addressed.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Looks good so far!

Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

Nice!

runtime/src/trace.rs Outdated Show resolved Hide resolved
runtime/src/trace.rs Show resolved Hide resolved
runtime/src/trace.rs Show resolved Hide resolved
runtime/src/trace.rs Outdated Show resolved Hide resolved
runtime/src/trace.rs Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's a focused summary of the latest updates:

The PR adds OpenTelemetry support to shuttle-runtime with a new feature flag and dependency updates in Cargo.toml.

  • Added new setup-telemetry feature flag that's independent from setup-tracing for better control over telemetry capabilities
  • Pinned opentelemetry_sdk to version 0.27.1 but has version mismatch with tracing-opentelemetry at v0.28.0 that needs resolution
  • Removed unused dependencies tokio-util and uuid for better maintenance

The changes show good progress on telemetry integration but require version alignment between OpenTelemetry dependencies.

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

runtime/Cargo.toml Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's a focused summary of the latest updates:

Improved error handling and field value processing in OpenTelemetry integration for shuttle-runtime.

  • Added robust error handling in ProviderGuard::drop() with proper logging for shutdown failures
  • Implemented SpanFieldVisitor trait with efficient field value handling for better performance
  • Standardized log attribute names through EventFieldValues field remapping system

The changes demonstrate good attention to error handling and performance while maintaining clean code organization.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@the-wondersmith the-wondersmith force-pushed the feat/user-project-telemetry branch from 0239e57 to 656c9dd Compare January 20, 2025 14:12
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's a focused summary of the latest updates:

  • Added SpanFieldVisitor trait and implementations in trace.rs for efficient field value handling in OpenTelemetry integration
  • Improved error handling in ProviderGuard::drop() with proper logging for telemetry component shutdowns
  • Introduced field remapping in EventFieldValues to standardize log attribute names across the system

The changes demonstrate good attention to performance optimization and error handling while maintaining clean code organization.

Note: I've kept this summary brief and focused only on the most recent changes not mentioned in previous reviews, as requested.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@the-wondersmith the-wondersmith merged commit 1b478d5 into main Jan 20, 2025
35 checks passed
@the-wondersmith the-wondersmith deleted the feat/user-project-telemetry branch January 20, 2025 14:39
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