feat: project telemetry config models #1972
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces telemetry configuration models to support external telemetry sink integrations with Betterstack, Datadog, and Grafana Cloud services.
- Added
telemetry.rswithProjectTelemetrySinkConfigenum for handling different telemetry provider configurations - Implemented
From<Vec<ProjectTelemetrySinkConfig>>trait for safe display representation inProjectTelemetryConfigResponse - Added TypeScript type definitions in
types.tswith corresponding interfaces and discriminated unions for telemetry configurations
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR updates the telemetry configuration models with improved field accessibility and enum handling patterns.
- Made configuration fields public (
pub) inBetterstackConfig,DatadogConfig, andGrafanaCloudConfigstructs for external access - Added
strum::AsRefStrderive macro toProjectTelemetrySinkConfigenum for string conversion support - Simplified enum pattern matching from
{ .. }to(_)for cleaner code inFromimplementation
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR adds the final touches to the telemetry configuration models with improved type safety and database integration.
- Added
#[typeshare::typeshare]attribute to all telemetry structs and enums for TypeScript type generation - Implemented
as_db_type()method onProjectTelemetrySinkConfigfor database type string generation - Added
TelemetrySinkStatusstruct withenabledfield for safe status representation
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| pub struct GrafanaCloudConfig { | ||
| pub token: String, | ||
| pub endpoint: String, | ||
| pub instance_id: String, |
There was a problem hiding this comment.
style: Consider adding validation for the endpoint URL and instance_id format.
There was a problem hiding this comment.
Validation can happen in backend handler if needed
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR updates the telemetry configuration models to ensure consistent naming conventions between Rust and TypeScript.
- Changed TypeScript type discriminators in
types.tsfrom PascalCase to snake_case for consistency with Rust enums - Added
#[serde(rename_all = "snake_case")]to ensure consistent serialization across languages - Added
#[strum(serialize_all = "snake_case")]for string conversion consistency
2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
common/types.ts
Outdated
| /** The user-supplied config required to export telemetry to a given external sink */ | ||
| export type ProjectTelemetrySinkConfig = | ||
| /** [Betterstack](https://betterstack.com/docs/logs/open-telemetry/) */ | ||
| | { type: "betterstack", content: BetterstackConfig } | ||
| /** [Datadog](https://docs.datadoghq.com/opentelemetry/collector_exporter/otel_collector_datadog_exporter) */ | ||
| | { type: "datadog", content: DatadogConfig } | ||
| /** [Grafana Cloud](https://grafana.com/docs/grafana-cloud/send-data/otlp/) */ | ||
| | { type: "grafana_cloud", content: GrafanaCloudConfig }; |
There was a problem hiding this comment.
logic: Breaking change: changing type discriminators from PascalCase to snake_case will break existing API consumers. Consider versioning the API or coordinating with consumers before deploying.
| pub fn as_db_type(&self) -> String { | ||
| format!("project::telemetry::{}::config", self.as_ref()) | ||
| } |
There was a problem hiding this comment.
logic: as_db_type could panic if as_ref() returns an unexpected value. Consider using a match statement for exhaustive pattern matching.
| pub fn as_db_type(&self) -> String { | |
| format!("project::telemetry::{}::config", self.as_ref()) | |
| } | |
| pub fn as_db_type(&self) -> String { | |
| let sink_type = match self { | |
| Self::Betterstack(_) => "betterstack", | |
| Self::Datadog(_) => "datadog", | |
| Self::GrafanaCloud(_) => "grafana_cloud", | |
| }; | |
| format!("project::telemetry::{}::config", sink_type) | |
| } |
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR updates the typeshare-cli version to 1.13.0 across build configurations to support the new telemetry models.
- Updated typeshare-cli version from 1.11.0 to 1.13.0 in
Makefile.tomlfor local development - Updated typeshare-cli version in
.circleci/config.ymlfor CI/CD pipeline consistency - Regenerated
types.tswith updated typeshare version to ensure type compatibility
4 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -109,7 +109,7 @@ command = "git-cliff" | |||
| args = ["-o", "CHANGELOG.md", "-t", "${@}"] | |||
|
|
|||
| [tasks.types] | |||
| install_crate = { crate_name = "typeshare-cli", binary = "typeshare", test_arg = ["-V"], version = "1.11.0" } | |||
| install_crate = { crate_name = "typeshare-cli", binary = "typeshare", test_arg = ["-V"], version = "1.13.0" } | |||
There was a problem hiding this comment.
style: Consider pinning to exact version with =1.13.0 to prevent unintended upgrades
| @@ -173,7 +173,7 @@ jobs: | |||
| - restore-cargo-and-sccache | |||
| - install-cargo-make | |||
| - run: cargo make ci-workspace | |||
| - run: curl --proto '=https' --tlsv1.2 -LsSf https://github.com/1Password/typeshare/releases/download/v1.11.0/typeshare-cli-v1.11.0-installer.sh | sh | |||
| - run: curl --proto '=https' --tlsv1.2 -LsSf https://github.com/1Password/typeshare/releases/download/v1.13.0/typeshare-cli-v1.13.0-installer.sh | sh | |||
There was a problem hiding this comment.
style: consider pinning the installer script hash for security
| #[derive(Eq, Clone, PartialEq, Serialize, Deserialize)] | ||
| #[typeshare::typeshare] | ||
| pub struct BetterstackConfig { | ||
| pub source_token: String, | ||
| } |
There was a problem hiding this comment.
style: Consider adding validation for the source_token format and non-emptiness
common/src/models/telemetry.rs
Outdated
| #[test] | ||
| fn sink_config_enum() { | ||
| assert_eq!( | ||
| "betterstack", | ||
| ProjectTelemetrySinkConfig::Betterstack(BetterstackConfig { | ||
| source_token: "".into() | ||
| }) | ||
| .as_ref() | ||
| ); | ||
| assert_eq!( | ||
| "project::telemetry::betterstack::config", | ||
| ProjectTelemetrySinkConfig::Betterstack(BetterstackConfig { | ||
| source_token: "".into() | ||
| }) | ||
| .as_db_type() | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| "betterstack", | ||
| ProjectTelemetrySinkConfigDiscriminants::Betterstack.as_ref() | ||
| ); | ||
| assert_eq!( | ||
| "grafana_cloud", | ||
| ProjectTelemetrySinkConfigDiscriminants::GrafanaCloud.as_ref() | ||
| ); | ||
| assert_eq!( | ||
| "\"betterstack\"", | ||
| serde_json::to_string(&ProjectTelemetrySinkConfigDiscriminants::Betterstack).unwrap() | ||
| ); | ||
| assert_eq!( | ||
| "\"grafana_cloud\"", | ||
| serde_json::to_string(&ProjectTelemetrySinkConfigDiscriminants::GrafanaCloud).unwrap() | ||
| ); | ||
| } |
There was a problem hiding this comment.
style: Add test cases for Datadog variant to ensure complete coverage of all enum variants
| #[test] | |
| fn sink_config_enum() { | |
| assert_eq!( | |
| "betterstack", | |
| ProjectTelemetrySinkConfig::Betterstack(BetterstackConfig { | |
| source_token: "".into() | |
| }) | |
| .as_ref() | |
| ); | |
| assert_eq!( | |
| "project::telemetry::betterstack::config", | |
| ProjectTelemetrySinkConfig::Betterstack(BetterstackConfig { | |
| source_token: "".into() | |
| }) | |
| .as_db_type() | |
| ); | |
| assert_eq!( | |
| "betterstack", | |
| ProjectTelemetrySinkConfigDiscriminants::Betterstack.as_ref() | |
| ); | |
| assert_eq!( | |
| "grafana_cloud", | |
| ProjectTelemetrySinkConfigDiscriminants::GrafanaCloud.as_ref() | |
| ); | |
| assert_eq!( | |
| "\"betterstack\"", | |
| serde_json::to_string(&ProjectTelemetrySinkConfigDiscriminants::Betterstack).unwrap() | |
| ); | |
| assert_eq!( | |
| "\"grafana_cloud\"", | |
| serde_json::to_string(&ProjectTelemetrySinkConfigDiscriminants::GrafanaCloud).unwrap() | |
| ); | |
| } | |
| #[test] | |
| fn sink_config_enum() { | |
| assert_eq!( | |
| "betterstack", | |
| ProjectTelemetrySinkConfig::Betterstack(BetterstackConfig { | |
| source_token: "".into() | |
| }) | |
| .as_ref() | |
| ); | |
| assert_eq!( | |
| "project::telemetry::betterstack::config", | |
| ProjectTelemetrySinkConfig::Betterstack(BetterstackConfig { | |
| source_token: "".into() | |
| }) | |
| .as_db_type() | |
| ); | |
| assert_eq!( | |
| "datadog", | |
| ProjectTelemetrySinkConfig::Datadog(DatadogConfig { | |
| api_key: "".into() | |
| }) | |
| .as_ref() | |
| ); | |
| assert_eq!( | |
| "project::telemetry::datadog::config", | |
| ProjectTelemetrySinkConfig::Datadog(DatadogConfig { | |
| api_key: "".into() | |
| }) | |
| .as_db_type() | |
| ); | |
| assert_eq!( | |
| "betterstack", | |
| ProjectTelemetrySinkConfigDiscriminants::Betterstack.as_ref() | |
| ); | |
| assert_eq!( | |
| "grafana_cloud", | |
| ProjectTelemetrySinkConfigDiscriminants::GrafanaCloud.as_ref() | |
| ); | |
| assert_eq!( | |
| "\"betterstack\"", | |
| serde_json::to_string(&ProjectTelemetrySinkConfigDiscriminants::Betterstack).unwrap() | |
| ); | |
| assert_eq!( | |
| "\"grafana_cloud\"", | |
| serde_json::to_string(&ProjectTelemetrySinkConfigDiscriminants::GrafanaCloud).unwrap() | |
| ); | |
| } |
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR simplifies telemetry type names by removing redundant 'Project' prefixes while maintaining functionality.
- Renamed
ProjectTelemetryConfigResponsetoTelemetryConfigResponsein both Rust and TypeScript - Renamed
ProjectTelemetrySinkConfigtoTelemetrySinkConfigfor cleaner naming - Updated type references in
telemetry.rsandtypes.tsto reflect new names
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| /** The user-supplied config required to export telemetry to a given external sink */ | ||
| export type TelemetrySinkConfig = | ||
| /** [Betterstack](https://betterstack.com/docs/logs/open-telemetry/) */ | ||
| | { type: "betterstack", content: BetterstackConfig } | ||
| /** [Datadog](https://docs.datadoghq.com/opentelemetry/collector_exporter/otel_collector_datadog_exporter) */ | ||
| | { type: "datadog", content: DatadogConfig } | ||
| /** [Grafana Cloud](https://grafana.com/docs/grafana-cloud/send-data/otlp/) */ | ||
| | { type: "grafana_cloud", content: GrafanaCloudConfig }; |
There was a problem hiding this comment.
style: Consider adding a discriminant type alias for TelemetrySinkConfig (e.g., export type TelemetrySinkConfigType = 'betterstack' | 'datadog' | 'grafana_cloud') to make it easier for consumers to work with the type field
No description provided.