chore: Refactored zlib instrumentation to be subscriber based - #4233
chore: Refactored zlib instrumentation to be subscriber based#4233jsumners-nr wants to merge 1 commit into
zlib instrumentation to be subscriber based#4233Conversation
762e9d4 to
52ac80a
Compare
jsumners-nr
left a comment
jsumners-nr
left a comment
There was a problem hiding this comment.
At this time, I have not explored uncovered variations of the zlib API. I am opening the PR in its most basic state so that we can discuss the ideas in it before finalizing our approach. Once we do that, I will revisit updating API coverage.
| /** | ||
| * The underlying local storage object. | ||
| * | ||
| * @see https://nodejs.org/api/async_context.html#class-asynclocalstorage | ||
| * @returns {AsyncLocalStorage} | ||
| */ | ||
| get store() { | ||
| return this._asyncLocalStorage | ||
| } |
There was a problem hiding this comment.
I get that accessing _ prefixed fields within our code base is acceptable and the actual intention of those fields. But I think, even within our own code base, it is better to expose things via public fields. It gives users confidence that they are doing the right thing instead of the anxiety induced by utilizing conventionally private fields.
| Object.defineProperties(Tracer.prototype, { | ||
| /** | ||
| * Retrieve the AsyncLocalStorage instance. | ||
| * | ||
| * @see https://nodejs.org/api/async_context.html#class-asynclocalstorage | ||
| * @returns {AsyncLocalStorage} | ||
| */ | ||
| asyncStore: { | ||
| get () { | ||
| return this._contextManager.store | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Just bubbling up the underlying getter to the interface we typically access the storage object from.
There was a problem hiding this comment.
I'm not sure if we really need this. This comes from the work I was experimenting with around @platformatic/kafka in October of 2025. My memory of that work is hazy at best. I think my idea was to give a base to inherit from that doesn't come with any implementation detail baggage, and then refactor the existing ones atop it (e.g. dc-base and db).
There was a problem hiding this comment.
This also comes from my experimentation with @platformatic/kafka, but it has been tweaked to actually do stuff. This smaller task helped me narrow in on how these classes should work. In short:
TracingChannelSubscriptiondescribes the event handlers an instrumentation will require. By defining only the event methods the instrumentation needs, we don't need to update something likethis.events.TracingChannelSubscriberutilizes one, or many, instances ofTracingChannelSubscriptionto register event listeners and manage segment life cycles.
There was a problem hiding this comment.
I ported the shimmer wrap methods to these simple utilities. It's likely that I missed some obscure detail of the shimmer implementation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4233 +/- ##
==========================================
- Coverage 97.52% 97.38% -0.14%
==========================================
Files 521 525 +4
Lines 62940 63543 +603
Branches 1 1
==========================================
+ Hits 61380 61883 +503
- Misses 1560 1660 +100
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
This PR resolves #4222.