refactor: Add attributable protocol for typed attribute values #7077
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7077 +/- ##
========================================
Coverage ? 84.846%
========================================
Files ? 461
Lines ? 27816
Branches ? 12334
========================================
Hits ? 23601
Misses ? 4171
Partials ? 44
Continue to review full report in Codecov by Sentry.
|
| @@ -1,5 +1,5 @@ | |||
| protocol BatcherItem: Encodable { | |||
| var attributes: [String: SentryAttribute] { get set } | |||
| var attributesMap: [String: SentryAttributeValue] { get set } | |||
There was a problem hiding this comment.
This was renamed because it would otherwise collide with the type of SentryAttribute.attributes
philprime
requested review from
itaybre,
noahsmartin and
philipphofmann
as code owners
| } | ||
|
|
||
|
|
||
| /// Logs a trace-level message with structured string interpolation and optional attributes. |
There was a problem hiding this comment.
These newly introduced logger methods are not strictly necessary but can be beneficial for SDK users due to type hints. We still need the type-erased method to keep supporting Objective-C but eventually we should deprecate the Any-typed version for Swift and have an Objective-C wrapper
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 854ca12 | 1219.94 ms | 1251.32 ms | 31.38 ms |
| 2675d3c | 1218.94 ms | 1239.36 ms | 20.42 ms |
| 6ad363f | 1196.04 ms | 1223.27 ms | 27.23 ms |
| 87fb58a | 1233.12 ms | 1257.17 ms | 24.04 ms |
| 449d185 | 1216.31 ms | 1251.94 ms | 35.62 ms |
| d3f650a | 1225.45 ms | 1241.86 ms | 16.41 ms |
| 99104c9 | 1224.84 ms | 1247.08 ms | 22.24 ms |
| 0f410ad | 1193.34 ms | 1255.49 ms | 62.15 ms |
| f8029e2 | 1245.16 ms | 1261.32 ms | 16.16 ms |
| e3ebff3 | 1223.47 ms | 1249.27 ms | 25.80 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 854ca12 | 23.74 KiB | 996.96 KiB | 973.22 KiB |
| 2675d3c | 23.75 KiB | 928.16 KiB | 904.41 KiB |
| 6ad363f | 23.75 KiB | 1022.66 KiB | 998.91 KiB |
| 87fb58a | 23.75 KiB | 919.91 KiB | 896.16 KiB |
| 449d185 | 23.75 KiB | 980.81 KiB | 957.06 KiB |
| d3f650a | 23.75 KiB | 902.48 KiB | 878.73 KiB |
| 99104c9 | 23.75 KiB | 894.83 KiB | 871.09 KiB |
| 0f410ad | 24.14 KiB | 1.01 MiB | 1014.82 KiB |
| f8029e2 | 23.75 KiB | 893.72 KiB | 869.97 KiB |
| e3ebff3 | 23.75 KiB | 878.48 KiB | 854.73 KiB |
|
Array handling is incomplete, moved back to draft |
… corresponding unit tests
| func getContextForKey(_ key: String) -> [String: Any]? | ||
|
|
||
| /// List of attributes with erased value type for compatibility with public ``Scope``. | ||
| var attributes: [String: Any] { get } |
There was a problem hiding this comment.
Should we add @deprecated?
There was a problem hiding this comment.
Not sure why we would want to do that. It's not deprecated, the attributesMap is just because for the Metrics API it won't be the type [String: Any]
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
I really like that we're adding type safety to the attributes with this PR. I think we need some slight tweaking for the public logging API to better communicate which types our users can use, such as:
public func warn(_ message: SentryLogMessage, attributes: [String: SentryAttributeValuable] = [:])
|
|
||
| func testEncodeStringArrayAttribute() throws { | ||
| // -- Arrange -- | ||
| let attribute = SentryLog.Attribute(stringArray: ["hello", "world", "test"]) |
There was a problem hiding this comment.
l: As we're not only going to use this for logs but also for metrics, I would rather use this naming in the tests
| let attribute = SentryLog.Attribute(stringArray: ["hello", "world", "test"]) | |
| let attribute = SentryAttribute(stringArray: ["hello", "world", "test"]) |
| public init(stringArray values: [String]) { | ||
| self.type = "string[]" | ||
| self.value = values | ||
| super.init() | ||
| } |
There was a problem hiding this comment.
h: I would highly appreciate proper code docs for all these new init methods, as these are public API.
| } | ||
|
|
||
| public init(booleanArray values: [Bool]) { | ||
| self.type = "boolean[]" |
There was a problem hiding this comment.
m: What about defining an internal string-based enum for all the allowed types? So something like
private enum AttributeType: String {
case stringType = "string",
booleanType = "boolean",
integerType = "integer",
integerArrayType = "integer[]",
doubleType = "double",
doubleArrayType = "double[]"
}SentryAttributeValue could also use it for the types.
| @testable import Sentry | ||
| import XCTest | ||
|
|
||
| final class SentryAttributeValuableTests: XCTestCase { |
There was a problem hiding this comment.
Thanks for the clean test coverage.
| @@ -0,0 +1,145 @@ | |||
| public protocol SentryAttributeValuable { | |||
| var asAttributeValue: SentryAttributeValue { get } | |||
There was a problem hiding this comment.
m: We have sentry in the naming everywhere. I think this would make it a bit clearer that this returns a SentryAttributeValue.
| var asAttributeValue: SentryAttributeValue { get } | |
| var asSentryAttributeValue: SentryAttributeValue { get } |
| }) | ||
| } | ||
|
|
||
| private func castArrayToAttributeValue(values: [SentryAttributeValuable]) -> SentryAttributeValue { |
There was a problem hiding this comment.
m: Honestly, I find that a bit confusing. I can do this, and the compiler doesn't complain. When looking at this code, I could assume that we support arrays with different types for the attributes, but instead, we silently convert them to a string array. Instead, it would be great if the following code wouldn't even compile.
let attribute : [SentryAttributeValuable] = [1,2,3, 3.3, "test"]
let attributeValue = attribute.asAttributeValue
There was a problem hiding this comment.
I tried to do that but it's not possible in Swift to have multiple conformances like this:
extension Array: SentryAttributeValuable where Element == Bool {
public var asAttributeValue: SentryAttributeValue {
return .booleanArray(self)
}
}
extension Array: SentryAttributeValuable where Element == Int {
public var asAttributeValue: SentryAttributeValue {
return .integerArray(self)
}
}
extension Array: SentryAttributeValuable where Element == String {
public var asAttributeValue: SentryAttributeValue {
return .stringArray(self)
}
}
extension Array: SentryAttributeValuable where Element == Double {
public var asAttributeValue: SentryAttributeValue {
return .doubleArray(self)
}
}...because Swift does not allow multiple conditional conformances of the same generic type to the same protocol — even if the constraints are mutually exclusive. This is a hard compiler rule, not a limitation of my code.
If you, @itaybre or @noahsmartin know how we can make only these types of arrays can actually be casted to SentryAttributeValuable, I happily integrate it. I've already tried it before and couldn't find a way.
There was a problem hiding this comment.
OK, maybe we need a completely different approach if it doesn't work with the current one. I can try to come up with something, but that will take some time, and that will have to wait after the holidays.
| public func trace(_ message: SentryLogMessage, attributes: [String: SentryAttributeValuable] = [:]) { | ||
| captureLog(level: .trace, logMessage: message, attributes: attributes) | ||
| } |
There was a problem hiding this comment.
h: I'm all in favor of adding type safety. This is the API our users interact with most, and currently, this approach could do a better job of communicating to our users which types they can use. When I declare the attributes, I see SentryAttributeValuable, which I find a bit confusing. What is SentryAttributeValuable? Which types can I use?
OK, so this works:
let attributes: [String: SentryAttributeValuable] = ["retry_policy": "exponential"])]
This leads to two compiler errors, which is excellent:
let attributes: [String: SentryAttributeValuable] = ["string": "hello", "double": 0.0, "int": 1, "bool": true, "array": [1,2,3, 3.3], "dict": ["hello": "you"], "null": NSNull()]
But if I do this, the code compiles. It should also fail to compile here. It doesn't because it falls back to using [String: Any] for the attributes.
SentrySDK.logger.warn(logString, attributes: ["string": "hello", "double": 0.0, "int": 1, "bool": true, "array": [1,2,3, 3.3], "dict": ["hello": "you"], "null": NSNull()])
There was a problem hiding this comment.
That's what I am afraid off, we can't really offer this API for Logs, because it's so ambiguous with Any. I actually fee like we shouldn't introduce this for Logs right now and instead when we have a Objective-C wrapper framework.
There was a problem hiding this comment.
We could make it work by using a different name for attributes such as typeSafeAttributes, or typedAttributes and even consider using _ typeSafeAttributes.
public func warn(_ message: SentryLogMessage, _ typeSafeAttributes: [String: SentryAttributeValuable] = [:]) {
And I think we could rename SentryAttributeValuable to SentryTypedAttribute or SentryTypeSafeAttribute.
📜 Description
This PR introduces a protocol-based system for typed attribute values to improve type safety when working with structured logging attributes. The changes add:
SentryAttributeValuethat represents attribute values (string, boolean, integer, double, and their array variants)SentryAttributeValuablethat allows types to convert themselves toSentryAttributeValueString,Bool,Int,Double, andFloatto conform toSentryAttributeValuableThis change refactors the Objective-C compatible
SentryAttributeto use the Swift enumSentryAttributeValueas its backing type. It also updatesBatcherScopeto useSentryAttributeValuedirectly instead ofSentryAttributeobjects.This PR also introduces support for Array attribute types as defined in the documentation.
Changes in Public API:
The currently proposed changes contain new public API endpoints for the
SentryLoggerto accept[String: SentryAttributeValuable]instead of[String: Any]for improved type safety. I acknowledge this is a breaking change, therefore I am open to remove it until the next major version.If we keep it it could cause ambiguity issues because we have to method signatures both with the same parameters, only having different Dictionary types as the attributes.
The main use case are the new methods introduced by Metrics in #6957
💡 Motivation and Context
Previously, attributes were stored as
[String: Any]dictionaries, which provided no compile-time type safety. This made it easy to accidentally pass unsupported types or incorrect values, which are converted toStringas a fallback, sending unpredictable data. By introducing a protocol-based system withSentryAttributeValuable, we can:SentryAttributeobjects (using thExpressibleBy...protocol extensions)SentryAttributeclass still exists and can convert fromSentryAttributeValuefor Objective-C interop.I also need this change to implement #6957.
💚 How did you test it?
BatcherScopeTests,BatcherTests, andSentryLoggerTeststo use the new typed attribute system📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.