-
Notifications
You must be signed in to change notification settings - Fork 84
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
GEN-196: Move error-stack-experimental
into error-stack
#5181
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.
Rationale makes sense. Legal approval-only here. Tim for UAT.
error-stack-experimental
into error-stack
error-stack-experimental
into error-stack
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5181 +/- ##
==========================================
+ Coverage 18.12% 19.72% +1.60%
==========================================
Files 506 510 +4
Lines 16776 17182 +406
Branches 2546 2546
==========================================
+ Hits 3041 3390 +349
- Misses 13723 13780 +57
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Mainly a few comments/questions. Looks very good, excited to test this out!
/// # Unstable Feature | ||
/// | ||
/// This trait is currently available only under the `unstable` feature flag and | ||
/// does not adhere to semver guarantees. Its API may change in future releases. |
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 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 just wanted to make sure to mention that it is exempt from semver guarantees and why, not necessarily that it's unstable.
/// } | ||
/// # Ok(()) | ||
/// ``` | ||
#[cfg(feature = "unstable")] |
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 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've liked if there's a way to conditionally disable match arms that would circumvent the problem. Sadly I didn't find an obvious ergonomic way to do so. I'll just disable the unstable macro during generation.
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 noticed, that the bail
macro is probably not working as expected:
error[E0034]: multiple applicable items in scope
--> libs/error-stack/tests/test_macros.rs:36:55
|
36 | let report = capture_error(|| bail!([RootError])).attach_printable(PrintableA(0));
| ^^^^^^^^^^^^^^^^ multiple `attach_printable` found
|
= note: candidate #1 is defined in an impl for the type `error_stack::Report<C>`
= note: candidate #2 is defined in an impl for the type `error_stack::Report<[C]>`
Probably an issue for a follow-up (-> non-blocking)
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 tried around with it a bit and failed to properly generate a doc here. We might want to remove the current bail
from the docs and add a bail
macro with both variants and a notice that the second variant is unstable
.
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 noticed this as well, I wanted to do this in a follow up - because the code for Report<[C]>
and Report<C>
is actually the same now (previously the return type was different)
/// By moving this check to runtime, `Bomb` ensures that `ReportSink` is properly | ||
/// consumed. Depending on its configuration, it will either: | ||
/// - Panic if the `ReportSink` is dropped without being used (when set to `Panic` mode) | ||
/// - Emit a warning to stderr (when in `Warn` mode, which is the default) | ||
/// - Do nothing if properly defused (i.e., when `ReportSink` is correctly used) |
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.
Do we want to move the documentation to the variants instead?
pub fn add(&mut self, report: impl Into<Report<[C]>>) { | ||
let report = report.into(); | ||
|
||
match self.report.as_mut() { | ||
Some(existing) => existing.append(report), | ||
None => self.report = Some(report), | ||
} | ||
} |
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.
add
is a term which is rarely used in Rust. Maybe we want to use append
or the Extend
trait? I don't have a strong opinion on that, I just want to mention it 🙂
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.
Thought about that as well but didn't rename it because of the follow up PR that applies the changes to the worktree. I was thinking of renaming it to either "push" or "append" (more likely append) to be in line with the existing methods in Report<[C]>.
What di you think about append?
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.
As I proposed append
myself I'm fine with that 🚀
We can always change it later, no rush 👍🏻
/// ``` | ||
/// | ||
/// [`finish`]: ReportSink::finish | ||
pub fn finish_with_value<T>(mut self, ok: T) -> Result<T, Report<[C]>> { |
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.
Non-blocking: Similar to bool.then_some
we could use finish_ok()
.
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.
A few things we like to do in follow-up but nothing blocking this PR from being merged 🚀
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.
Rationale makes sense. Legal approval-only here. Tim for UAT.
Legal re-approval
65d144d
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.
Rationale makes sense. Legal approval-only here. Tim for UAT.
Legal re-approval
bc779f7
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.
Rationale makes sense. Legal approval-only here. Tim for UAT.
Legal re-approval
Benchmark results
|
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 100 entities | Flame Graph | |
entity_by_id | 1000 entities | Flame Graph | |
entity_by_id | 10000 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
scaling_read_entity_complete_zero_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
representative_read_entity
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph |
representative_read_entity_type
Function | Value | Mean | Flame graphs |
---|---|---|---|
get_entity_type_by_id | Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579
|
Flame Graph |
scaling_read_entity_complete_one_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
representative_read_multiple_entities
Function | Value | Mean | Flame graphs |
---|---|---|---|
link_by_source_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph |
🌟 What is the purpose of this PR?
This moves
error-stack-experimental
into theerror-stack
crate, reason for this is that we didn't plan on publishingerror-stack-experimental
anyway, this allows us to use the experimental features, while also giving it to users under a feature flag and makes developing these unstable features easier.The only problem is that this means that
error-stack
unstable
features won't be semver compatible anymore, but we judged that that was a good tradeoff to be made.This PR also replaces the
ResultMultiExt
with aReportSink
, sadly due tomust_use
limitations we cannot solely rely onmust_use
to ensure the value is consumed, insteadReportSink
makes use of runtime validation (which is only active duringdebug_assertions
).ReportSink::new()
will create a sink that will stderr if the sink isn't finished,Report::new_armed
goes a bit further and will panic. This ensures that the value really is used every single time, but is quite disruptive, so is not the default.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
turbo.json
's have been updated to reflect this