-
Notifications
You must be signed in to change notification settings - Fork 114
feat: add telemetry via sentry #2865
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
commit: |
@@ -0,0 +1,39 @@ | |||
use uuid::Uuid; | |||
|
|||
const SENTRY_URL: &str = "https://7602663e43cb9dee8c42d1e5e70293f8@o4504307129188352.ingest.us.sentry.io/4509962797252608"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Concern: The Sentry DSN URL contains an authentication token (7602663e43cb9dee8c42d1e5e70293f8
) that's hardcoded in the source code. This creates a security risk as anyone with repository access can view this token.
Recommendation: Move this sensitive credential to a secure configuration mechanism:
- Store in environment variables
- Use a secrets management system
- Add to a configuration file that's excluded from version control
This follows security best practices for credential management and prevents potential misuse of the Sentry instance.
const SENTRY_URL: &str = "https://[email protected].sentry.io/4509962797252608"; | |
const SENTRY_URL: &str = option_env!("SENTRY_DSN").unwrap_or("https://public@sentry.example.com/1"); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
let status = status_code; | ||
let group = error.as_ref().map_or("-", |x| &x.group); | ||
let code = error.as_ref().map_or("-", |x| &x.code); | ||
let meta = error.as_ref().and_then(|x| x.metadata.as_ref()).unwrap_or(&serde_json::Value::Null); | ||
let internal = error.as_ref().and_then(|x| x.internal.as_ref()).map_or("-", |x| x.as_ref()); | ||
|
||
tracing::error!( | ||
status = ?status_code, | ||
group = %error.as_ref().map_or("-", |x| &x.group), | ||
code = ?error.as_ref().map_or("-", |x| &x.code), | ||
meta = %error.as_ref().and_then(|x| x.metadata.as_ref()).unwrap_or(&serde_json::Value::Null), | ||
internal = %error.as_ref().and_then(|x| x.internal.as_ref()).map_or("-", |x| x.as_ref()), | ||
?status, | ||
%group, | ||
%code, | ||
%meta, | ||
%internal, | ||
"http server error" | ||
); | ||
|
||
sentry::with_scope( | ||
|scope| { | ||
scope.set_tag("status", status_code); | ||
scope.set_tag("group", group); | ||
scope.set_tag("code", code); | ||
scope.set_tag("meta", meta); | ||
scope.set_tag("internal", internal); | ||
}, | ||
|| { | ||
sentry::capture_message("http server error", sentry::Level::Error); | ||
}, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the error data extraction logic into a helper function. The same pattern appears in both server error handling (lines 131-136) and client error handling (lines 159-163), creating duplication that could lead to inconsistencies if one section is updated but not the other.
A simple function like:
fn extract_error_data(error: &Option<ErrorResponse>) -> (String, String, &serde_json::Value, String) {
let group = error.as_ref().map_or("-", |x| &x.group);
let code = error.as_ref().map_or("-", |x| &x.code);
let meta = error.as_ref().and_then(|x| x.metadata.as_ref()).unwrap_or(&serde_json::Value::Null);
let internal = error.as_ref().and_then(|x| x.internal.as_ref()).map_or("-", |x| x.as_ref());
(group.to_string(), code.to_string(), meta, internal.to_string())
}
This would improve maintainability and ensure consistent error data extraction across both error handling paths.
let status = status_code; | |
let group = error.as_ref().map_or("-", |x| &x.group); | |
let code = error.as_ref().map_or("-", |x| &x.code); | |
let meta = error.as_ref().and_then(|x| x.metadata.as_ref()).unwrap_or(&serde_json::Value::Null); | |
let internal = error.as_ref().and_then(|x| x.internal.as_ref()).map_or("-", |x| x.as_ref()); | |
tracing::error!( | |
status = ?status_code, | |
group = %error.as_ref().map_or("-", |x| &x.group), | |
code = ?error.as_ref().map_or("-", |x| &x.code), | |
meta = %error.as_ref().and_then(|x| x.metadata.as_ref()).unwrap_or(&serde_json::Value::Null), | |
internal = %error.as_ref().and_then(|x| x.internal.as_ref()).map_or("-", |x| x.as_ref()), | |
?status, | |
%group, | |
%code, | |
%meta, | |
%internal, | |
"http server error" | |
); | |
sentry::with_scope( | |
|scope| { | |
scope.set_tag("status", status_code); | |
scope.set_tag("group", group); | |
scope.set_tag("code", code); | |
scope.set_tag("meta", meta); | |
scope.set_tag("internal", internal); | |
}, | |
|| { | |
sentry::capture_message("http server error", sentry::Level::Error); | |
}, | |
); | |
let status = status_code; | |
let (group, code, meta, internal) = extract_error_data(&error); | |
tracing::error!( | |
?status, | |
%group, | |
%code, | |
%meta, | |
%internal, | |
"http server error" | |
); | |
sentry::with_scope( | |
|scope| { | |
scope.set_tag("status", status_code); | |
scope.set_tag("group", &group); | |
scope.set_tag("code", &code); | |
scope.set_tag("meta", meta); | |
scope.set_tag("internal", &internal); | |
}, | |
|| { | |
sentry::capture_message("http server error", sentry::Level::Error); | |
}, | |
); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
scope.set_tag("status", status_code); | ||
scope.set_tag("group", group); | ||
scope.set_tag("code", code); | ||
scope.set_tag("meta", meta); | ||
scope.set_tag("internal", internal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be a type inconsistency in how the status code is handled. On line 131, status_code
is assigned to variable status
, but on line 148, status_code
is used directly when setting the Sentry tag.
For proper Sentry tag serialization, consider using the string representation of the status code:
scope.set_tag("status", status.to_string());
This ensures the status code is properly serialized as a string value when sent to Sentry, rather than potentially passing the StatusCode enum directly.
scope.set_tag("status", status_code); | |
scope.set_tag("group", group); | |
scope.set_tag("code", code); | |
scope.set_tag("meta", meta); | |
scope.set_tag("internal", internal); | |
scope.set_tag("status", status.to_string()); | |
scope.set_tag("group", group); | |
scope.set_tag("code", code); | |
scope.set_tag("meta", meta); | |
scope.set_tag("internal", internal); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
fn default() -> Self { | ||
// NOTE: Telemetry is opt-out | ||
Telemetry { enabled: true } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Privacy and Security Consideration: The current implementation makes telemetry opt-out rather than opt-in (enabled: true
by default). This approach may lead to unintended data collection from users who haven't explicitly consented to telemetry. Consider changing to an opt-in model where enabled
defaults to false
, requiring users to make a conscious choice to enable data collection. This would better align with privacy best practices and reduce the risk of inadvertent data sharing.
fn default() -> Self { | |
// NOTE: Telemetry is opt-out | |
Telemetry { enabled: true } | |
} | |
fn default() -> Self { | |
// NOTE: Telemetry is opt-in | |
Telemetry { enabled: false } | |
} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
7e50154
to
109863b
Compare
Claude encountered an error —— View job I'll analyze this and get back to you. |
109863b
to
3bcc74f
Compare
Claude encountered an error —— View job I'll analyze this and get back to you. |
Claude encountered an error —— View job I'll analyze this and get back to you. |
6cf9da6
to
624b560
Compare
Merge activity
|
No description provided.