chore: (qe) Preparatory refactoring to introduce log capturing. #3617
Conversation
miguelff
force-pushed
the
integration/log-capture-prep-refactoring
branch
2 times, most recently
from
fffa84e to
6970951
Compare
| if req.method() == Method::POST && req.uri().path().contains("transaction") { | ||
| return handle_transaction(state, req).await; | ||
| if req.method() == Method::POST && req.uri().path().starts_with("/transaction") { | ||
| return transaction_handler(state, req).await; | ||
| } | ||
|
|
||
| if [Method::POST, Method::GET].contains(req.method()) | ||
| && req.uri().path().contains("metrics") | ||
| && req.uri().path().starts_with("/metrics") | ||
| && state.enable_metrics | ||
| { | ||
| return handle_metrics(state, req).await; | ||
| return metrics_handler(state, req).await; | ||
| } |
There was a problem hiding this comment.
I hope all consumers properly shaped their URLs 😬
| impl PrismaResponse { | ||
| pub fn errors(&self) -> Vec<&GQLError> { | ||
| match self { | ||
| PrismaResponse::Single(ref s) => s.errors().collect(), | ||
| PrismaResponse::Multi(ref m) => m.errors().collect(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I suppose this was dead code?
There was a problem hiding this comment.
Yes, there isn't any dynamic call to a particular prisma response
| } | ||
|
|
||
| meta.target() == "quaint::connector::metrics" && meta.name() == "quaint:query" | ||
| } |
There was a problem hiding this comment.
I see a lot of pub fn items in this file and elsewhere, and that is wholesale reexported as public API to consumers of the crate. Does this all need to be public? It make the API of a crate smaller and easier to understand if internal items are pub(crate) instead.
There was a problem hiding this comment.
Right, I didn't restrict visibility of any of those, and just moved it. I will check and restrict when possible.
|
|
||
| impl State { | ||
| /// Create a new instance of `State`. | ||
| fn new(cx: PrismaContext, enable_playground: bool, enable_debug_mode: bool, enable_metrics: bool) -> Self { |
There was a problem hiding this comment.
No need to change this now, but boolean params are very poor in information when you read callers and really easy to get wrong (which boolean goes at which position). In my experience bitflags with enumflags are a lot easier to read when you construct them (all values are named), on top of being more compact in memory (here it would be 1 byte instead of 3). They're also more code and from a library, so, tradeoffs.
There was a problem hiding this comment.
I totally agree, grabed the whole State object from a previous patch in progress, will try to address in here.
There was a problem hiding this comment.
Not blocking, like the rest of my feedback here, so do what you feel most comfortable with. No need to learn a whole (small) library for this if you haven't used enumflags2 before.
There was a problem hiding this comment.
I like the approach of using enum2flags for modeling a compacted set of flags. FWIW, these do already exist as boolean flags coming from the CLI. so we would have to make a transformation. Nevertheless, I don't want to add yet another boolean to this struct in case we have more flags, so I'm ok with exploring this option. Thanks for the suggestion.
I will do it, though, in a different PR after I merge both this and #3618 to prevent more conflicts, as the #3618 has changes using the state.
|
(none of my feedback is blocking) |
|
Thank you both @Weakky and @tomhoule for the sensible feedback, it also gives me a lot of good information that I didn't know about and that would help improve the patch, which is the best outcome possible for a review. Thank you so much! 🤗 Will follow up with a new commit addressing the feedback and merge if all good. |
…uld not change the existing behavior of the engine
miguelff
force-pushed
the
integration/log-capture-prep-refactoring
branch
from
ca0e1b0 to
d4977fb
Compare
miguelff
force-pushed
the
integration/log-capture-prep-refactoring
branch
from
d4977fb to
dbd7ac1
Compare
Tracked in https://github.com/prisma/client-planning/issues/158
Extracted from #3505 and rebased.
To be released before #3618
This PR does a preparatory refactoring for the log capturing feature that can be reviewed and be released independently