Update mobc and metrics crates #5015
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CodSpeed Performance ReportMerging #5015 will not alter performanceComparing Summary
|
WASM Query Engine file Size
|
aqrln
reviewed
Oct 10, 2024
SevInf
force-pushed
the
mobc-and-metrics-up
branch
3 times, most recently
from
65fdda5
to
3f44866
Compare
aqrln
force-pushed
the
mobc-and-metrics-up
branch
from
f01e7ed
to
9b37e2b
Compare
aqrln
added a commit
that referenced
this pull request
Oct 23, 2024
After updating mobc in #5015, we faced some test failures due to previous bugs unmasking others. For some reason filtering the events emitted by the metrics to tracing bridge didn't quite work as expected, and some of the callsites (like the decrement method for counters) were disabled the first time they were registered with the subscriber, unless the interest cache was rebuilt right before emitting the event, making decrementing the counters not work. Aside from the fixed mobc issues, this is another reason behind issues like prisma/prisma#25177. We tried debugging it with Serhii before he left but we eventually agreed that it would be better to just get rid of the hack with carrying the metrics data through traces. Now, the reason this hack was necessary was because we always had a global metrics recorder but we somehow needed to isolate the metrics by the engine instance in the library engine, and have a way to associate the emitted metrics with the correct engine implicitly based on the async context. Tracing conveniently has all of that implemented, but `metrics` crate provides no instrumentation for futures. Luckily, it does have a way to run a (synchronous) closure overriding the global recorder for this closure (or without a global recorder at all), and this is enough of a building block to implement the necessary instrumentation ourselves, which is exactly what this PR does! Now we have simpler, more robust and more efficient setup for metrics that is not coupled with tracing, no global recorder in Node-API engine and in tests, and we still support a global recorder in the binary engine just in case (and to avoid some boilerplate the library engine has to have for both tracing and metrics in request handlers). Closes: prisma/team-orm#1317
mobc update includes fixes from importcjj/mobc#99 Rest of the PR is update to new api of the `metrics` crate. Close prisma/team-orm#1317
This reverts commit b83d79f.
aqrln
force-pushed
the
mobc-and-metrics-up
branch
from
258fb1a
to
f9c052a
Compare
aqrln
added a commit
that referenced
this pull request
Oct 23, 2024
After updating mobc in #5015, we faced some test failures due to previous bugs unmasking others. For some reason filtering the events emitted by the metrics to tracing bridge didn't quite work as expected, and some of the callsites (like the decrement method for counters) were disabled the first time they were registered with the subscriber, unless the interest cache was rebuilt right before emitting the event, making decrementing the counters not work. Aside from the fixed mobc issues, this is another reason behind issues like prisma/prisma#25177. We tried debugging it with Serhii before he left but we eventually agreed that it would be better to just get rid of the hack with carrying the metrics data through traces. Now, the reason this hack was necessary was because we always had a global metrics recorder but we somehow needed to isolate the metrics by the engine instance in the library engine, and have a way to associate the emitted metrics with the correct engine implicitly based on the async context. Tracing conveniently has all of that implemented, but `metrics` crate provides no instrumentation for futures. Luckily, it does have a way to run a (synchronous) closure overriding the global recorder for this closure (or without a global recorder at all), and this is enough of a building block to implement the necessary instrumentation ourselves, which is exactly what this PR does! Now we have simpler, more robust and more efficient setup for metrics that is not coupled with tracing, no global recorder in Node-API engine and in tests, and we still support a global recorder in the binary engine just in case (and to avoid some boilerplate the library engine has to have for both tracing and metrics in request handlers). Closes: prisma/team-orm#1317
aqrln
reviewed
Oct 23, 2024
| @@ -9,4 +9,4 @@ proc-macro = true | |||
| [dependencies] | |||
| proc-macro2 = "1.0.26" | |||
| quote = "1.0.2" | |||
| syn = "1.0.5" | |||
| syn = { version = "1.0.5", features = ["full"] } | |||
There was a problem hiding this comment.
Story behind this: we use types gated behind the full feature in this crate but we didn't specify this feature here. We also depended on something elsewhere in the workspace which transitively depended on syn with this feature enabled, so we got this feature enabled here via cargo feature unification, but now that the transitive dependency is gone after updating crates, this crate stopped compiling as we started getting exactly what we were asking for in dependencies.
aqrln
approved these changes
Oct 24, 2024
|
Failing tests are fixed in #5023 |
jkomyno
approved these changes
Oct 24, 2024
aqrln
added a commit
that referenced
this pull request
Oct 24, 2024
After updating mobc in #5015, we faced some test failures due to previous bugs unmasking others. For some reason filtering the events emitted by the metrics to tracing bridge didn't quite work as expected, and some of the callsites (like the decrement method for counters) were disabled the first time they were registered with the subscriber, unless the interest cache was rebuilt right before emitting the event, making decrementing the counters not work. Aside from the fixed mobc issues, this is another reason behind issues like prisma/prisma#25177. We tried debugging it with Serhii before he left but we eventually agreed that it would be better to just get rid of the hack with carrying the metrics data through traces. Now, the reason this hack was necessary was because we always had a global metrics recorder but we somehow needed to isolate the metrics by the engine instance in the library engine, and have a way to associate the emitted metrics with the correct engine implicitly based on the async context. Tracing conveniently has all of that implemented, but `metrics` crate provides no instrumentation for futures. Luckily, it does have a way to run a (synchronous) closure overriding the global recorder for this closure (or without a global recorder at all), and this is enough of a building block to implement the necessary instrumentation ourselves, which is exactly what this PR does! Now we have simpler, more robust and more efficient setup for metrics that is not coupled with tracing, no global recorder in Node-API engine and in tests, and we still support a global recorder in the binary engine just in case (and to avoid some boilerplate the library engine has to have for both tracing and metrics in request handlers). Closes: prisma/team-orm#1317
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
mobc update includes fixes from importcjj/mobc#99
Rest of the PR is update to new api of the
metricscrate.Ref: prisma/team-orm#1317