-
Notifications
You must be signed in to change notification settings - Fork 518
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(instrumentation-runtime-node)!: add prom-client-metrics #2136
feat(instrumentation-runtime-node)!: add prom-client-metrics #2136
Conversation
…arbage collector, heap size and heap space
|
* @default 5000 | ||
*/ | ||
eventLoopUtilizationMeasurementInterval?: number; | ||
monitoringPrecision?: number; |
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.
curious about this change, can you give more context why the rename?
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.
Because right now i use it for every metrics, not only for eventLoopUtilization. You think I needed to add parameters for every new metrics group ?
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.
No need
I am super excited for this one, I think it was the main thing missing from OpenTelemetry's Node.js instrumentation! Thanks for opening this PR. |
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.
Can you please take a look at open-telemetry/semantic-conventions#991 and give your input there? It looks like you made some different decisions than what was made there and I want to make sure we're all on the same page before merging something.
Thanks. Yes, i have some mismatches. |
@pikalovArtemN I would love to hear your opinion on the mismatches. Let me know what you think makes more sense 😄 |
I have mismatches in names like eventloop => event_loop, and i need to fix units, i totaly forgot to set it properly XD. And fix attributes. I decided to devide the metrics not by label, but names like it was in the prom-client library, but i think you do a better solution in the convension. I think we need to colobarate, not all of metrics and attributes that i added exists in convention such as:
|
I can add that
I can add that
I think it would be good, I got that from the prom-client metrics list, so should be possible
I can make those changes on the convention
I think it would be good, I also got that from prom-client, but it doesn't have |
heads up: because the semantic convention could be for all JS runtime (not just nodejs, but also others such as denojs or bunjs), it was suggested to not use |
Ok |
If it is about any js runtime the node.js specific parts should be removed/renamed/...
|
I don't understand what you mean here. For things that are node specific, we can still create the metrics for it, such as the active libuv requests, I just need remove that from the semantic convention, but this PR can still keep it because can be useful. |
There are JS engines (e.g. quickjs) which use a different heap structure/gc then google v8 (the JS engine used by node.js or deno). As a result I'm not sure if GC metrics should have v8 in the name to allow to distinguish them from other engines. |
I think right now we need to concentrate on nodejs realization only, because Node is most popular and in the next iteration adopt instrumentation to work with different engines. Right now it's so much work to do |
now I've noticed the Please get the latest version of |
…tion' into feat/node/prom-client-implementation
@dyladan you still have a request here, which was already addressed, can you take a look again? |
@dyladan is seems you need to approve since previously requested changes in this PR. |
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
export const V8_HEAP_SIZE_NAME_ATTRIBUTE = 'heap.space.name'; |
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 assume these are just temporary until the next semconv release?
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.
actually, they already exist on the current release (they didn't when the PR was created), so @pikalovArtemN you can replace for the final values. For example, this one is ATTR_V8JS_HEAP_SPACE_NAME
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.
fix
@pikalovArtemN this is almost done :) could you fix the conflicts and address the last comment from @maryliag? Thank you |
@@ -62,7 +62,7 @@ nodejs_performance_event_loop_utilization 0.010140079547955264 | |||
|
|||
| name | type | unit | default | description | | |||
|---|---|---|---|---| | |||
| [`eventLoopUtilizationMeasurementInterval`](./src/types.ts#L25) | `int` | millisecond | `5000` | The approximate number of milliseconds for which to calculate event loop utilization averages. A larger value will result in more accurate averages at the expense of less granular data. Should be set to below the scrape interval of your metrics collector to avoid duplicated data points. | | |||
| [`monitoringPrecision`](./src/types.ts#L25) | `int` | millisecond | `5000` | The approximate number of milliseconds for which to calculate event loop utilization averages. A larger value will result in more accurate averages at the expense of less granular data. Should be set to below the scrape interval of your metrics collector to avoid duplicated data points. | |
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.
suggestion(non-blocking): update the default
You changed the default to 10
so we should update the README too
| [`monitoringPrecision`](./src/types.ts#L25) | `int` | millisecond | `5000` | The approximate number of milliseconds for which to calculate event loop utilization averages. A larger value will result in more accurate averages at the expense of less granular data. Should be set to below the scrape interval of your metrics collector to avoid duplicated data points. | | |
| [`monitoringPrecision`](./src/types.ts#L25) | `int` | millisecond | `10` | The approximate number of milliseconds for which to calculate event loop utilization averages. A larger value will result in more accurate averages at the expense of less granular data. Should be set to below the scrape interval of your metrics collector to avoid duplicated data points. | |
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.
Fixed in 6b5fc8b
@david-luna any idea how fast this can be released after it is merged? |
…tion' into feat/node/prom-client-implementation
@kristian240 after this gets merged, we can aim for a new relase in a couple of days 😄 |
@@ -32,7 +32,7 @@ const prometheusExporter = new PrometheusExporter({ | |||
const sdk = new NodeSDK({ | |||
metricReader: prometheusExporter, | |||
instrumentations: [new RuntimeNodeInstrumentation({ | |||
eventLoopUtilizationMeasurementInterval: 5000, | |||
monitoringPrecision: 5000, |
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.
minor Q: It looks like the default is 10 ms (from lower down in this README). Should the example here show 10 rather than 5000?
Which problem is this PR solving?
Short description of the changes