-
Notifications
You must be signed in to change notification settings - Fork 39
update: docs + usage featuring latest improvements #139
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
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.
Some notes, overall great docs! Just need to flesh out a few confusing bits; the rest about what's allowed etc is great 👍
For general advice on how to use `Prometheus` make sure to also read the [Prometheus documentation][prometheus-docs]. | ||
- *Standards Compliant*: Follows Prometheus naming conventions and exposition formats, enforces base guarantees. | ||
- *Type Safe*: Prevents common configuration errors through Swift's type system. | ||
- *Thread Safe*: Operations use internal locking and conform to `Sendable`. |
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 guess we may stick to Swift buzzwords here, we usually say "concurrency safety", and I amended a bit the wording with sendable. On that note, We should enable Swift 6 mode if we want to claim concurrency safety, the compiler will do more checks then
- *Thread Safe*: Operations use internal locking and conform to `Sendable`. | |
- *Concurrency Safe*: Operations are thread safe, and safety is expressed using Swift's concurrency safety mechanisms such as `Sendable`. |
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.
Simplified.
|
||
## Thread Safety | ||
|
||
All operations are thread-safe, with the exception of the lower-level ``emit(into:)`` overload variant (compared to the thread-safe ``emitToBuffer()`` or ``emitToString()``). The client uses internal locking to ensure consistency across concurrent access and conforms to Swift's `Sendable` protocol, meaning it can be safely passed between actors and used in concurrent contexts without additional synchronization. |
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.
Why wouldn't emit(into:)
be thread safe, there's no reason for that 🤔
If you're concerned about someone concurrently calling with the same buffer -- swift won't allow that since that'd violate exclusive access with the inout enforces
I'd rephrase this section a bit, we don't need to explain Sendable to Swift developers here. Concurrency safety is kinda "expected" in Swift nowadays, however we should enable Swift 6 mode so we can prove we're 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.
Will check, let's get back to it next week. Meanwhile quickly rebased. In general I suppose I still live too much in the C and C++ world ...
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 mean that Swift will literarily prevent such races:
func test() async {
var buffer: [UInt8] = []
await Task {
emit(into: &buffer) // error: Sending value of non-Sendable type '() async -> ()' risks causing data races
}.value
await Task {
emit(into: &buffer)
}.value
}
while the following is OK:
func test() async {
var buffer: [UInt8] = []
await Task {
emit(into: &buffer)
}.value
}
because the value is "sent" to that task and cannot be accessed from other contexts.
In Swift we lean heavily on compiler assured concurrency safety and it would be weird to call out a method is not safe like that, because it's context dependent and the compiler will prevent the racy versions of code :-)
This is an error in Swift 6 mode (or "complete concurrency checking), and a warning in 5 mode; there's no need to call out in our docs issues that the compiler will prevent.
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.
Thank you! However, if it needs to be Sendable, you still need locking, just like we do with the internal buffer. I’ve rewritten this part of the documentation, so let me know!
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.
That's the thing -- there's no locking required if sendable analysis can prove it's not strictly needed or ensured otherwise -- see the capturing in the Task, or you could have a buffer in an actor and the collecting tasks be on the same actor etc... So yes, it needs to be safe and swift will enforce that but it doesn't explicitly mean a lock per se :)
8003cde
to
da902b5
Compare
Signed-off-by: Melissa Kilby <[email protected]>
More precise thread-safety guidance / documentation. Fix Swift Docc checks. Co-authored-by: Konrad `ktoso` Malawski <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>
da902b5
to
2c58e4f
Compare
Signed-off-by: Melissa Kilby <[email protected]>
2c58e4f
to
9c7e444
Compare
Looks good, nice updates @incertum ! |
update: docs featuring latest improvements
#135 should land first as I reference those updates.
CC @ktoso @heckj