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
chore: Refactor specs into classes for easier code navigation #2004
Conversation
4c3e2bc
to
22d62dd
Compare
22d62dd
to
f584382
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2004 +/- ##
==========================================
+ Coverage 97.03% 97.10% +0.07%
==========================================
Files 224 239 +15
Lines 40864 41020 +156
==========================================
+ Hits 39652 39834 +182
+ Misses 1212 1186 -26
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.
great work. hopefully this provides clarity for devs. also might be good to cut other tickets in a feature to update all instrumentation to make sure it uses a spec instead of returning a plain object that aligns with some spec. lastly, we talked in zoom but lib/shim/shim.js needs some cleanup on some of these redundant interface defintions.
|
||
/** | ||
* A callback invoked after an instrumented function has completed its work. | ||
* The instrumented function must have been invoked synchronously. |
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.
This applies to any type of instrumented function. It's just that the after function must be sync. it could have async operations in it but it won't do the right thing
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 you have some alternative text in mind? The current text is almost a verbatim copy of what existed in the original jsdoc.
b298b52
to
b04a1f6
Compare
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.
this is a great start. i linked 2 places where you couldn't find data types. we're close to landing this one
* Extra parameters which may be added to an operation or query segment. All of | ||
* these properties are optional. | ||
* | ||
* @typedef {object} DatastoreParameters |
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.
this is great. we may want to file a follow up as there are other parameters types for message queues, and webframework
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 know this was a lot of work but should help clarify a lot for engineers and using shim to instrument with our specs. very nice 👏🏻
Instrumentations utilize a "spec" object to provide a sort of DSL to the shimming library. There are several specific types of specs, which are documented at
shim.js:275
. There is also a set of utility functions for generating specs defined atspecs/index.js
. However, the utility methods are rarely used. The most prevalent pattern throughout the codebase is to create spec with object literals, utilizing hard gained knowledge about what a spec looks like and which one would be appropriate for the situation.My purpose here has been to research the how specs are utilized to determine which parts are actually used and codify them in easier to understand, and document, explicit objects.
This PR resolves issue #2006.