Skip to content
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

Editoast error management #237

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Editoast error management #237

wants to merge 13 commits into from

Conversation

leovalais
Copy link
Contributor

Settling on this topic is necessary in order to go forward with the split in crates.

@leovalais leovalais marked this pull request as ready for review September 18, 2024 09:27
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a first round, hope you like it. I'm really worried by the complexity of this macro and if there would be some other solution.

}
```

### Anonymous enums
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's such a QoL improvement when magic is around:

  • what will happen when types do not match and compiler errors start to kick in on variants that only exists in these EnumX types?

I believe that, indeed, all of these enums will be a lot of boilerplate code... but once written, not touch again and not a lot.

Also, I'm more for a incremental improvement. We only "think" it might be cumbersome... Let's not fix now, what might never be broken. Said differently, let's address QoL once we have identified a really problem.

I would drop this section personally.

Copy link
Contributor

@Khoyo Khoyo Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree. Writing error enums isn't fun, but with thiserror it isn't that cumbersome, and has the nice side effect of being usable without refering to same T2 variant...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point was to find a way to use them transparently by just using the implicit from() that goes with the ? operator. But after prototyping a bit I couldn't find a way to make that work with rust stable. anon_enum not implementing From either seems to confirm that. I kept the section with an explanation block on why we won't do it, just in case some Rust wizard comes by and magically find a solution... Cf. 717e31d (#237)

}
```

In order to be able to find and investigate the error later on, we associate to each `5xx` error a unique `incident` identifier. At first we'll just log the incident with:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear what the use for this unique identifier. I would understand to have one if I could relate to other places where this identifier is shown. If the idea is to have that identifier so we can go to Datadog to find the entire related trace, then maybe this identifier can be the trace ID directly?

Copy link
Contributor

@Khoyo Khoyo Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this identifier can be the trace ID directly

Do we have traceids when tracing isn't enabled? If not, maybe we should just specify it as some unique identifier, which may or may not be a trace id (and also, may or may not be a valid uuid too).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against using trace IDs if we can have them when telemetry is disabled.
I'm not sure if it's concerning or not, but should we decide to do so, we'd leak some trace IDs to the frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the discussion we had during the PIP, I moved this in the "Rejected ideas" section, I'll let you resolve if its ok 717e31d (#237)

- Dispose of a way to actually match on errors when they occur deeper in the stack
- Tie the errors to the endpoint they originate from in the OpenAPI.
- Separate the error definition and emission of their serailization.
- Statuate on how we want to forward Core's errors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have missed where we do rule on that in this document... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ec3e8d2 (#237)

Signed-off-by: Leo Valais <[email protected]>
Signed-off-by: Leo Valais <[email protected]>
@leovalais
Copy link
Contributor Author

Thanks for all the corrections! I'll get back to this document when I have some time, hopefully in the next iteration :)

@leovalais
Copy link
Contributor Author

I've added a bit of content, especially an implementation plan and details about how to forward Core errors.

Signed-off-by: Leo Valais <[email protected]>
@leovalais
Copy link
Contributor Author

Thanks for the corrections!

Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice introducing some new sections about "Issues of the old system" or "Implementation plan".

Signed-off-by: Leo Valais <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants