Skip to content
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

feat: Re-implement wrap-websocket into agent #1342

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ellisong
Copy link
Contributor

@ellisong ellisong commented Jan 23, 2025

Re-implement wrap-websocket code using ES6 extends syntax into the agent. This will allow supportability metrics to be reportable from websocket API usage.

Overview

We're re-implementing the wrap-websocket code into the agent that was taken out in 1.265.1 patch to address deploy issues. The new implementation uses ES6 extends syntax to include static class constants and all the properties of the original WebSocket object. Tests are included that cover usage with the third-party libraries robust-websocket and reconnecting-websocket.

Related Issue(s)

https://new-relic.atlassian.net/browse/NR-311646

Testing

Make sure tests pass.

@ellisong ellisong force-pushed the NR-311646-websocket branch from f5e39c5 to 68f9d1b Compare January 23, 2025 22:13
Copy link

Asset Size Report

Merging this pull request will result in the following asset size changes:

Agent Asset Previous Size New Size Diff
lite loader 29.77 kB / 10.79 kB (gzip) 30.84 kB / 11.19 kB (gzip) 3.59% / 3.68% (gzip)
lite async-chunk 52.59 kB / 17.08 kB (gzip) 53.61 kB / 17.46 kB (gzip) 1.94% / 2.25% (gzip)
pro loader 51.46 kB / 17.77 kB (gzip) 52.53 kB / 18.13 kB (gzip) 2.09% / 2.01% (gzip)
pro async-chunk 99.77 kB / 30.52 kB (gzip) 100.34 kB / 30.81 kB (gzip) 0.57% / 0.96% (gzip)
spa loader 59.08 kB / 20.09 kB (gzip) 60.15 kB / 20.45 kB (gzip) 1.82% / 1.79% (gzip)
spa async-chunk 114.49 kB / 34.85 kB (gzip) 115.06 kB / 35.14 kB (gzip) 0.5% / 0.84% (gzip)

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (45cb413) to head (68f9d1b).

Files with missing lines Patch % Lines
src/features/metrics/aggregate/index.js 66.66% 1 Missing ⚠️
src/features/metrics/instrument/index.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
+ Coverage   88.70%   88.82%   +0.12%     
==========================================
  Files         170      170              
  Lines        7331     7349      +18     
  Branches     1488     1487       -1     
==========================================
+ Hits         6503     6528      +25     
+ Misses        716      713       -3     
+ Partials      112      108       -4     
Flag Coverage Δ
integration-tests 91.23% <87.50%> (-0.01%) ⬇️
unit-tests 79.90% <83.33%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metal-messiah
Copy link
Member

General reminder that this is a high-risk feature. Should be deployed on its own and undergo more scrutiny before deployment

Copy link

github-actions bot commented Jan 24, 2025

Static Badge

Last ran on January 24, 2025 12:15:47 CST
Checking merge of (68f9d1b) into main (45cb413)

@metal-messiah
Copy link
Member

I will be pulling this in locally and testing some things when i wrap up my current dev work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a change from previous draft?

@cwli24
Copy link
Contributor

cwli24 commented Jan 24, 2025

PR description can be made more succinct; use the overview section for details. Example, "Tests are included for ..." are not relevant for customers on the release note summary view.

Also reminder to link the JIRA ticket in issues section.

@metal-messiah
Copy link
Member

@cwli24 lets coordinate a time to mob-test this if you dont mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants