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

Add node.js runtime metrics semantic conventions #991

Merged
merged 44 commits into from
Jun 28, 2024

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented May 1, 2024

Part Of #990

Changes

Adding semantic conventions for Node.js runtime metrics

Merge requirement checklist

@maryliag maryliag force-pushed the nodejs-metrics branch 2 times, most recently from 0415b17 to 8b98001 Compare May 1, 2024 13:38
@maryliag
Copy link
Contributor Author

maryliag commented May 1, 2024

I wasn't sure if I need to update something on the next, since there was no changes to existing semantic. This is a new one.

@trask
Copy link
Member

trask commented May 1, 2024

cc @open-telemetry/javascript-approvers

model/registry/nodejs.yaml Outdated Show resolved Hide resolved
model/registry/nodejs.yaml Outdated Show resolved Hide resolved
@maryliag maryliag marked this pull request as ready for review May 2, 2024 15:16
@maryliag maryliag requested review from a team as code owners May 2, 2024 15:16
@Netail
Copy link

Netail commented May 2, 2024

I am wondering whether or not it should be called NodeJS. What if someone uses Deno or Bun?

@maryliag maryliag force-pushed the nodejs-metrics branch 2 times, most recently from 50b741f to 5eb9c53 Compare May 2, 2024 20:39
@maryliag
Copy link
Contributor Author

maryliag commented May 3, 2024

I am wondering whether or not it should be called NodeJS. What if someone uses Deno or Bun?

what about just js ?

@Netail
Copy link

Netail commented May 3, 2024

I am wondering whether or not it should be called NodeJS. What if someone uses Deno or Bun?

what about just js ?

I mean I guess, but also feel like it can be confused with the language instead of the runtime environment

@maryliag
Copy link
Contributor Author

maryliag commented May 3, 2024

I mean I guess, but also feel like it can be confused with the language instead of the runtime environment

if we want to go be definition, it would be runtimejs

@Netail
Copy link

Netail commented May 3, 2024

I mean I guess, but also feel like it can be confused with the language instead of the runtime environment

if we want to go be definition, it would be runtimejs

jsruntime sounds maybe better? idk, might need some extra opinions

@maryliag maryliag force-pushed the nodejs-metrics branch 2 times, most recently from 12a8c9f to ce06636 Compare May 3, 2024 13:50
@pikalovArtemN
Copy link

pikalovArtemN commented May 3, 2024

For my opinion different runtimes need to have diferent preffix.
For example:
node - nodejs
bun - bun
deno - denojs
etc...
Or some attribute that specify what runtime is currently run.
Or metric idk.

Also i think for version we need to set metric like it does in the prom-client library. Attribute in all metrics with version, for my opinion, it's not a good solution.

@david-luna
Copy link

One last question from my side. I've checked that other metrics of time make use of histogram as instrument. Some examples:

Here all the metrics related to delay and using s as unit use the gauge instrument. Probably a naive question but why is that?

@maryliag
Copy link
Contributor Author

Here all the metrics related to delay and using s as unit use the gauge instrument. Probably a naive question but why is that?

That is a limitation of this type of metric for node, meaning, I'm using this more because is from the event loop and not because is a duration (I even use histogram on duration on other metrics).
The gist is that the values for min, max, p50, p90, p99 for these metrics come from the builtin histogram from the node runtime and is not possible to convert it to otel histogram, because we can only query the distribution not get the entire histogram object. This is why you will notice I have a separate metric for each value (min, max, etc), instead of just one called delay that would have everything.

Hope that helps clarify!

@david-luna
Copy link

Here all the metrics related to delay and using s as unit use the gauge instrument. Probably a naive question but why is that?

That is a limitation of this type of metric for node, meaning, I'm using this more because is from the event loop and not because is a duration (I even use histogram on duration on other metrics). The gist is that the values for min, max, p50, p90, p99 for these metrics come from the builtin histogram from the node runtime and is not possible to convert it to otel histogram, because we can only query the distribution not get the entire histogram object. This is why you will notice I have a separate metric for each value (min, max, etc), instead of just one called delay that would have everything.

Hope that helps clarify!

It helps a lot. Thank you! :)

@lmolkova
Copy link
Contributor

That is a limitation of this type of metric for node, meaning, I'm using this more because is from the event loop and not because is a duration (I even use histogram on duration on other metrics). The gist is that the values for min, max, p50, p90, p99 for these metrics come from the builtin histogram from the node runtime and is not possible to convert it to otel histogram, because we can only query the distribution not get the entire histogram object. This is why you will notice I have a separate metric for each value (min, max, etc), instead of just one called delay that would have everything.

Could you please add a similar note to the md file itself?

@open-telemetry/javascript-approvers any last minute feedback?

@maryliag
Copy link
Contributor Author

Could you please add a similar note to the md file itself?

Done

@lmolkova lmolkova merged commit 5077fd5 into open-telemetry:main Jun 28, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet