semantic-conventions icon indicating copy to clipboard operation
semantic-conventions copied to clipboard

Add node.js and v8 js engine runtime metrics semantic conventions

Open maryliag opened this issue 1 year ago • 12 comments
trafficstars

Fixes #990

Changes

Adding semantic conventions for:

  • Node.js runtime metrics
  • V8 Js Engine metrics

Merge requirement checklist

maryliag avatar May 01 '24 13:05 maryliag

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.

maryliag avatar May 01 '24 13:05 maryliag

cc @open-telemetry/javascript-approvers

trask avatar May 01 '24 14:05 trask

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

Netail avatar May 02 '24 20:05 Netail

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

what about just js ?

maryliag avatar May 03 '24 12:05 maryliag

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

Netail avatar May 03 '24 12:05 Netail

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

maryliag avatar May 03 '24 12:05 maryliag

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

Netail avatar May 03 '24 13:05 Netail

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.

pikalovArtemN avatar May 03 '24 18:05 pikalovArtemN

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

I think we should make it clear that it is only for NodeJS not other runtimes like deno, bun, or something else. JS is just a language but the underlying runtimes differ a lot. I would argue for something along the lines of nodejs, noderuntime, or nodejsruntime.

dyladan avatar May 07 '24 12:05 dyladan

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

I think we should make it clear that it is only for NodeJS not other runtimes like deno, bun, or something else. JS is just a language but the underlying runtimes differ a lot. I would argue for something along the lines of nodejs, noderuntime, or nodejsruntime.

It may be worth splitting the v8 semconv from the NodeJS semconv. The v8 engine is used by a few different JS runtimes and it would be worth making them consistent between runtimes. Most of the metrics returned by the v8 module are returned directly from the v8 engine such as getHeapStatistics. The node metrics returned by the perf_hooks module however, are specific to NodeJS.

dyladan avatar May 07 '24 13:05 dyladan

thank you for all the comments, I'm looking and will be replying to them 👀

maryliag avatar May 08 '24 13:05 maryliag

New updates:

  • Metrics separated in 2: node.js (containing only the node.js specific metrics), v8 js engine metrics (that can be used by other js runtime, such as deno)
  • replaced metrics where I was using type attributes to define the type, now they're separate metrics (for example, heap size and eventloop delay)

maryliag avatar May 08 '24 20:05 maryliag

From the discussion on the SIG meeting, keeping this PR just for node and created a new one for the v8 on https://github.com/open-telemetry/semantic-conventions/pull/1066

maryliag avatar May 23 '24 18:05 maryliag

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

  • https://github.com/open-telemetry/semantic-conventions/blob/cde003cd371ef8cc802f275bcfed15cff8fd6a48/model/metrics/jvm-metrics.yaml#L49
  • https://github.com/open-telemetry/semantic-conventions/blob/cde003cd371ef8cc802f275bcfed15cff8fd6a48/model/metrics/go-metrics.yaml#L77

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

david-luna avatar Jun 18 '24 11:06 david-luna

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!

maryliag avatar Jun 18 '24 12:06 maryliag

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! :)

david-luna avatar Jun 21 '24 08:06 david-luna

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?

lmolkova avatar Jun 28 '24 03:06 lmolkova

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

Done

maryliag avatar Jun 28 '24 15:06 maryliag