fix: Add dsc project id to all spans#6011
Conversation
| attributes.insert(SENTRY__DSC__TRANSACTION, transaction.clone()); | ||
| } | ||
| if let Some(project_id) = project_id { | ||
| attributes.insert(SENTRY__DSC__PROJECT_ID, project_id.value() as i64); |
There was a problem hiding this comment.
Do project ids always fit into i64? If yes, I think we should change ProjectId(u64) to ProjectId(i64) (because implementing From<u64> for AttributeValue doesn't make sense as EAP doesn't support it right?).
There was a problem hiding this comment.
Then we should store it as a string instead and update conventions accordingly (also confirm with Simon).
There was a problem hiding this comment.
Do you mean that if project ids don't always fit in i64, we should store them as strings? But if they always fit, it's fine to just change to ProjectId(i64) (and leave conventions as is)?
There was a problem hiding this comment.
No, we should always store it as a string. It's typed as a u64 we have no other guarantee that it will always fit in an i64, so we should use a format to store that's not gonna make us problems -> string.
elramen
force-pushed
the
elramen-ds
branch
2 times, most recently
from
9615a36 to
3e0de84
Compare
| attributes.insert(SENTRY__DSC__TRANSACTION, transaction.clone()); | ||
| } | ||
| if let Some(project_id) = project_id { | ||
| attributes.insert(SENTRY__DSC__PROJECT_ID, project_id.value() as i64); |
There was a problem hiding this comment.
No, we should always store it as a string. It's typed as a u64 we have no other guarantee that it will always fit in an i64, so we should use a format to store that's not gonna make us problems -> string.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 67703e4. Configure here.
| attributes.insert(SENTRY__DSC__TRANSACTION, transaction.clone()); | ||
| } | ||
| if let Some(project_id) = sampling_project_id { | ||
| attributes.insert(SENTRY__DSC__PROJECT_ID, project_id.value() as i64); |
There was a problem hiding this comment.
Lossy u64-to-i64 cast for project ID
Medium Severity
ProjectId::value() returns u64, and casting it to i64 with as silently wraps values exceeding i64::MAX. While current project IDs are small, ProjectId is typed as u64 with no guarantee it fits in i64. The PR discussion explicitly concluded this attribute needs to be stored as a string to avoid data corruption, but the code still uses as i64.
Reviewed by Cursor Bugbot for commit 67703e4. Configure here.
| SENTRY__DSC__PROJECT_ID.to_owned(), | ||
| Annotated::new(Value::U64(project_id.value())), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inconsistent project ID type between span formats
Medium Severity
The sentry.dsc.project_id attribute is stored as Value::U64 in legacy/v1 spans but as i64 (typed as AttributeType::Integer) in EAP/v2 spans. The same logical value has different representations across span formats, which can cause inconsistent behavior in downstream consumers expecting a uniform type.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 67703e4. Configure here.
2 participants
Needed for the new dynamic sampling pipeline.
Fixes: https://linear.app/getsentry/issue/RELAY-239/add-dsc-attributes-trace-root-project-onto-every-span
Fixes: https://linear.app/getsentry/issue/RELAY-244/add-project-id-to-transaction-spans
Fixes: https://linear.app/getsentry/issue/RELAY-245/add-project-id-to-v2-spans
Fixes: https://linear.app/getsentry/issue/RELAY-243/add-project-id-to-v1-spans