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

New: Add prefix support to be able to specify a custom Prometheous metric name prefix #14

Open
wants to merge 4 commits into
base: v2
Choose a base branch
from

Conversation

DataTables
Copy link

@DataTables DataTables commented Oct 27, 2020

#12

I've done this on the v2 branch since that seems to be were the development is - I hope that is okay.

Copy link
Contributor

@naueramant naueramant left a comment

Choose a reason for hiding this comment

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

The pull request contains the dist/ directory which should not be included.

@naueramant
Copy link
Contributor

I think this code might be a better fit for the v2 branch. Would you please look into adding this to v2 instead?

Also thanks a lot for you interest in the project!

@DataTables
Copy link
Author

Hi - thanks for your replies! This was targeting the v2 branch - have I not set the pull request up properly to reflect that?

Regarding the dist directory - the package.json references "main": "./dist/index.js",. So to be able to npm install this package and then use it, the dist directory would need to be included. The source is TS, so it couldn't be directly referenced from package.json and have it work without tsc.

@naueramant
Copy link
Contributor

My bad you did indeed target the v2 branch!

The package is meant the be build and published to npm so this repo does itself not hold the build.

@DataTables
Copy link
Author

Sorry for quite getting it - when you npm install socket.io-prometheus-metrics it will pull in the contents of this repo (I'm assuming the v2 branch here) - then if you do a require('socket.io-prometheus-metrics') it will fail since dist/main.js isn't present.

@naueramant
Copy link
Contributor

naueramant commented Nov 19, 2020

No, when running npm install the client fetches a tarball from the npm registry with the released package and extracts it into node_modules/<package name>.

You can find links to the tarball and inspect it yourself by running npm view as so:

npm view socket.io-prometheus-metrics

[email protected] | Apache-2.0 | deps: 3 | versions: 7
Prometheus metrics for socket.io

keywords: prometheus, socket.io, metrics

dist
.tarball: https://registry.npmjs.org/socket.io-prometheus-metrics/-/socket.io-prometheus-metrics-1.0.6.tgz
.shasum: 59189d57daaa879708a76d6618cc0bbef87fc9a0
.integrity: sha512-rG1XIWuFRysVE/VE2ikJcYbv1ZqcY5IZ5CJOoih2MMiVx7GujwvxF1olcpDlQ/iGGryCXC/FRoBAB00Qnzsntw==
.unpackedSize: 24.3 kB

dependencies:
express: ^4.16.4     prom-client: ^11.2.1 socket.io: ^2.2.0    

maintainers:
- jenkinsuniwise <[email protected]>
- jonastranberguniwise <[email protected]>
- mads_stenhoj <[email protected]>

dist-tags:
latest: 1.0.6  

published 3 months ago by jonastranberguniwise <[email protected]>

When releasing a package you do the following:

  1. build the packge to generate the dist dir
  2. run npm publish which will bundle all the files (including the dist folder) in a tar and push it to the npm registry.

Thats why there is no need for the dist folder in version control. In general compiled code should not be version controlled.

Doesn't support SocketIO 1 and 2 any more.
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.

2 participants