-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fix breaking changes introduced by newest opentelemetry update #152
Fix breaking changes introduced by newest opentelemetry update #152
Conversation
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.
See comment in PR about missing clear method.
const counter = metricService.getValueRecorder('test1'); | ||
counter.clear(); | ||
const counter = metricService.getHistogram('test1'); | ||
// counter.clear(); |
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.
Clear doesn't exist anymore on histogram
const counter = metricService.getValueRecorder('test1', { description: 'test1 description' }); | ||
counter.clear(); | ||
const counter = metricService.getHistogram('test1', { description: 'test1 description' }); | ||
// counter.clear(); |
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.
Clear doesn't exist anymore on histogram
ae1e68d
to
88100e7
Compare
Converted this to a DRAFT for now, will check if everything works out later. |
ping @adrianmxb Do you need any help with this PR? |
@pragmaticivan really stressful week, will look into it hopefully beginning of next week. Got some issues with metrics. |
@pragmaticivan The dependency on opentelemetry-node-metrics seems to be a problem. |
Hi @adrianmxb I've pushed some changes in the mainstream. I added you as co-author here 6ed63ae Will only release once https://www.npmjs.com/package/@opentelemetry/host-metrics gets the latest update to 0.27.0 |
@pragmaticivan did you see the referenced issue i linked in #151 ? |
Hi @adrianmxb I'm actually considering dropping this library you commented in order to use https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/packages/opentelemetry-host-metrics only. Which is maintained by open telemetry js team. The reason is that this package doesn't seems to provide types or doesn't actually have versions in the package. |
This PR closes #151 .
I followed through the Upgrade guidelines of opentelemetry-js, some things are looking a bit different.
Should be a good starting point though.
I had to change the spec for the api-metrics middleware test, the .clear() function seems to be missing in the new otel api-metrics spec.