-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(py): Expose DataCategory via C-ABI #651
Conversation
py/sentry_relay/consts.py
Outdated
Returns categories that count as traditional error tracking events. | ||
""" | ||
return frozenset( | ||
DataCategory.DEFAULT, DataCategory.ERROR, DataCategory.SECURITY |
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.
Note that this changes compared to Sentry, where SECURITY
was not included:
This is mainly used by quotas, which will now include security events as originally intended.
SpanStatus::Unimplemented => write!(f, "unimplemented"), | ||
SpanStatus::Unavailable => write!(f, "unavailable"), | ||
SpanStatus::InternalError => write!(f, "internal_error"), | ||
SpanStatus::Unknown => write!(f, "unknown"), |
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.
This is now changed from "unknown_error"
, along with all other uses.
runtime_props.misc_primary_cpu_brand = None; | ||
#[allow(deprecated)] | ||
{ | ||
runtime_props.misc_primary_cpu_brand = None; |
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.
This is fixes an existing warning, unrelated to this patch.
if not attr.startswith(prefix): | ||
continue | ||
|
||
status_name = attr[len(prefix) :].lower() |
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.
I would say this is actually safe - per convention the constants must always match the default serialization. That said, if we ever diverge from this in the future (which we should not!) we can call into an API for this.
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.
Yes this seems safe.
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.
I think the changes to the default serialization of SpanStatus should be deployed with extreme care.
if not attr.startswith(prefix): | ||
continue | ||
|
||
status_name = attr[len(prefix) :].lower() |
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.
Yes this seems safe.
Exposes
DataCategory
via the C-ABI to Python for consumption in Sentry and Snuba. Along with this, also exposes theSpanStatus
, still via the old dictionary. In a future change, this could be made an enum, too.To expose the constants, they have been moved to
relay-cabi
. Unfortunately, a limitation in cbindgen does not allow to parserelay-general
directly. On the upside, this creates a central place for constants that are exposed from Relay to other services in Sentry.Along with this, the deprecated serialization of span status
"unknown_error"
is removed. By now, all downstream consumers have been updated.