-
Notifications
You must be signed in to change notification settings - Fork 468
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
base: main
Are you sure you want to change the base?
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 ?
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 |
…tion' into feat/node/prom-client-implementation
@maryliag I update code in convention style, check please |
this._config, | ||
ConventionalNamePrefix.NodeJsRuntime | ||
), | ||
new GCCollector(this._config, ConventionalNamePrefix.V8EnjineRuntime), |
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.
type, enjine -> engine
max: this.checkNan(this._histogram.max / 1e9), | ||
mean: this.checkNan(this._histogram.mean / 1e9), | ||
stddev: this.checkNan(this._histogram.stddev / 1e9), | ||
p50: this.checkNan(this._histogram.percentile(90) / 1e9), |
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.
this._histogram.percentile(90)
-> this._histogram.percentile(50)
*/ | ||
export enum ConventionalNamePrefix { | ||
NodeJsRuntime = 'nodejsruntime', | ||
V8EnjineRuntime = 'v8jsengineruntime', |
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.
typo: enjine -> engine
regarding the node js metrics, we're going with the shorter name, so just nodejs
without the runtime
.
For the v8 there is still deliberation on what name to use, with the possibility of using a generic jsengine
and have one of the attributes describing it is v8 in this case. Hopefully we can get to an agreement soon 🤞
description: 'Process heap space size available from Node.js in bytes.', | ||
}, | ||
[V8HeapSpaceMetrics.physical]: { | ||
description: 'Process heap space size available from Node.js in bytes.', |
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.
you're using the same description here as the available one
thank you for working on all those changes! |
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.
super excited and looking forward to use this in my projects. Any expectations on ETA?
private _reportEventloopLag(start: [number, number]): number { | ||
const delta = process.hrtime(start); | ||
const nanosec = delta[0] * 1e9 + delta[1]; | ||
return nanosec / 1e9; |
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.
question: Why not keep this ns
? Isn't process.hrtime
returning the second part because it can't be represented as seconds (within the first part)?
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.
Further to this, does it make sense to use the bigint version instead of the method used? The docs indicate this is legacy in node 22.
Added in node 10:
https://nodejs.org/docs/latest-v10.x/api/process.html#process_process_hrtime_bigint
Which problem is this PR solving?
Short description of the changes