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: add socket.io instrumentation #1284

Merged
merged 23 commits into from
Nov 22, 2022
Merged

Conversation

mottibec
Copy link
Contributor

@mottibec mottibec commented Nov 14, 2022

Upstream socket.io from ext-js repo to contrib.

The socket.io package has 5M weekly downloads on npm. The instrumentation has 4.6K weekly downloads on npm and it has been running in production environments for about 2 years with no issues.

I took the opportunity of changing the instrumentation package name and applied a few breaking changes which are documented in the README.md file.

The code itself is ported AS IS and can be improved in future PRs.

@mottibec mottibec requested a review from a team November 14, 2022 13:56
@github-actions github-actions bot requested review from obecny and rauno56 November 14, 2022 13:57
@mottibec mottibec changed the title Socketio feat: add socket.io instrumentation Nov 14, 2022
@mottibec mottibec marked this pull request as draft November 14, 2022 13:57
@mottibec mottibec marked this pull request as ready for review November 14, 2022 14:50
Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thanks!
added few nits

plugins/node/instrumentation-socket.io/README.md Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/package.json Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/src/types.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/src/socket.io.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/tsconfig.json Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/src/socket.io.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/src/socket.io.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/src/socket.io.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/src/socket.io.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-socket.io/src/socket.io.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #1284 (8542060) into main (fedb4a6) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1284      +/-   ##
==========================================
+ Coverage   96.08%   96.20%   +0.12%     
==========================================
  Files          14       15       +1     
  Lines         893      949      +56     
  Branches      191      195       +4     
==========================================
+ Hits          858      913      +55     
- Misses         35       36       +1     
Impacted Files Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts 98.21% <100.00%> (ø)

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Could we narrow down the versions we TAV-test to contain the CI time a bit?

Most popular versions currently:

┌─────────┬─────────┬──────────────┬─────────────────┬───────────┬──────────┐
│ (index) │ version │     time     │      tags       │ downloads │ ratio(%) │
├─────────┼─────────┼──────────────┼─────────────────┼───────────┼──────────┤
│    0    │ '2.4.0' │ '2021-01-04' │       []        │  723597   │   16.4   │
│    1    │ '2.5.0' │ '2022-06-26' │ [ 'v2-latest' ] │  618374   │    14    │
│    2    │ '4.5.3' │ '2022-10-15' │  [ 'latest' ]   │  464627   │   10.5   │
│    3    │ '2.1.1' │ '2018-05-17' │       []        │  401993   │   9.1    │
│    4    │ '2.3.0' │ '2019-09-20' │       []        │  330729   │   7.5    │
│    5    │ '4.5.1' │ '2022-05-17' │       []        │  286374   │   6.5    │
│    6    │ '3.1.3' │ '2021-03-12' │ [ 'v3-latest' ] │  263550   │    6     │
│    7    │ '4.4.1' │ '2022-01-06' │       []        │  174079   │   3.9    │
│    8    │ '4.5.2' │ '2022-09-02' │       []        │  148607   │   3.4    │
│    9    │ '2.2.0' │ '2018-11-28' │       []        │  143480   │   3.2    │
└─────────┴─────────┴──────────────┴─────────────────┴───────────┴──────────┘
Ratio of downloads through versions shown in the table: 80.43%
Total weekly downloads: 4,420,400

Please leave the ranges so that any new versions would also be included in testing. See tedious for an example.

@mottibec mottibec requested review from rauno56 and removed request for obecny November 16, 2022 13:23
@github-actions github-actions bot requested a review from obecny November 16, 2022 13:24
@rauno56 rauno56 merged commit f865143 into open-telemetry:main Nov 22, 2022
@dyladan dyladan mentioned this pull request Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants