-
Notifications
You must be signed in to change notification settings - Fork 1
Include the framework version with the report metadata #93
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
Conversation
| Judoscale.registerAdapter('judoscale-node', null, { | ||
| adapter_version: packageInfo.version, | ||
| language_version: process.version, | ||
| }) |
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.
Register node-core so we can report its version as well as the node version.
There's a bit of an inconsistency between Ruby/Node & Python here, in that Python only reports platform_version for everything, while Ruby/Node use language_version vs framework_version. I think that's something we should consider to make consistent across the board eventually.
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 agree
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 kinda like language_version & framework_version, but maybe we don't need that differentiation and can just make it like python does, because we know the version from the core packages represent the language and from the other specific packages it's the framework.
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.
adamlogic
left a comment
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.
👍 Thanks!!
| Judoscale.registerAdapter('judoscale-node', null, { | ||
| adapter_version: packageInfo.version, | ||
| language_version: process.version, | ||
| }) |
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 agree
This will report the fastify/express/bull/bullmq version they're using with each report, so we can more easily troubleshoot things if necessary and also verify version support on our side. This matches the behavior of Ruby/Python packages as well.
This allows us to report its adapter version & node version so we can keep track of versions supported, and it can aid troubleshooting. We need to make sure we skip trying to collect metrics from it though since there's no collector, so the report code was changed slightly to accommodate for that, and tests as well which were relying on the first registered adapter being the one they were testing against (which is no longer true.)
d4650e8 to
c6ef8cc
Compare
This will report the fastify/express/bull/bullmq version they're using with each report, as well as registering node-core as an adapter without a collector to report its version & node version, so we can more easily troubleshoot things if necessary and also verify version support on our side.
This matches the behavior of Ruby/Python packages as well.