fix(plugin-ioredis): end span on response from the server and set span status according to response #239
Conversation
- fix: end span on response from server, so duration accurately include the server processing
and networking time
- fix: set span status to error if server response with error
- feat: add responseHook to plugin config so user can register hook to add custom attributes to
span when successful response arrive from server
Codecov Report
@@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 95.41% 95.44% +0.02%
==========================================
Files 115 115
Lines 6090 6126 +36
Branches 590 597 +7
==========================================
+ Hits 5811 5847 +36
Misses 279 279
|
I would prefer if this was in a separate PR so it can be tracked in the changelog as a feature |
blumamir
changed the title
blumamir
changed the title
@dyladan @vmarchaud Done. |
|
Thanks @vmarchaud |
|
In contrib:
|
|
Yes the bug label makes this show up in the changelog. |
|
@blumamir pls fix the lint so I can merge it, thx |
|
The lint error is not related to my changes: When running locally on ioredis package I get no lint errors, but when running on the entire repo, it fails due to How can I make it pass? |
|
The branch passed until I merged master into this branch here |
|
@blumamir just run |
|
@blumamir weird, I can just suggest to do a full cleanup (when many packages changes, you have old package with lock etc. somethimes npm dosent update packages correctly). The easiest way is to remove all node_modules, then app package-lock.json, and all build folders, if you are on mac I have handy gist here to do it in one command you can just copy and paste in main package.json then run "npm run clean:all" and after this run "npm i" |
|
a wait |
|
seems like it might be the github actions , checking .... |
|
@blumamir just merged the latest to my PR |
|
@blumamir Could you rebase from master, i believe the previous lint error should be fixed ? |
@vmarchaud I updated the PR following the recent changes in StatusCode, and change to the test BTW, I see there is a PR for porting ioredis to the instrumentation API. Is there any point in merging this PR into master? I assume the |
In case this PR gets merged first I will update that one for the instrumentation package. I don't know if there is a concrete timeline when to remove the plugin packages. Regarding the lint error: I think you have to undo your change in graphql. I had a similar issue in the past and it was caused by different versions of installed npm packages lying around on my PC compared to that ones installed fresh in CI. Try to delete all package-lock.json files and node_modules folders and then do a fresh npm install in root folder of the project. |
Thanks @Flarna , that fixed the lint error :) |
We didn't discuss this i believe, however i think we aim for all core plugins to be ported (still grpc-js left but PR is open)
Just need one review from another maintainer /cc @obecny @dyladan |
|
@blumamir looks like tests for ioredis is broken, once fixed this can be merged |
I'll fix it now, thanks |
@obecny
@Flarna if I can assist somehow in merging this fix into the instrumentation PR, please let me know |
|
will merge this one first. the plugins will still remain for some time until all is moved to instrumentation, then will have to make more changes in core and then they can be deprecated and then finally remove. It will not happen in few days for sure. |
|
@blumamir I will copy over your changes into the instrumentation later today. In case I need some help I will ping you but it doesn't look that complicated. |
Which problem is this PR solving?
Short description of the changes
patch command
resolveandrejectfunctions. Only when the response arrives from the server - close the span (so the duration is accurate), and set status code to error if the server responds with error.