Add v8 js engine runtime metrics semantic conventions

[maryliag]

Part Of #990

Changes

Adding semantic conventions for V8 Js Engine metrics

Merge requirement checklist

• CONTRIBUTING.md guidelines followed.
• Change log entry added, according to the guidelines in When to add a changelog entry.
  • If your PR does not need a change log, start the PR title with [chore]
• schema-next.yaml updated with changes to existing conventions.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[maryliag]

friendly ping for reviews

[joaopgrassi]

Did a first pass, I will try to get more familiar with Nodejs v8 and then look at it again.

[joaopgrassi]

Press enter too soon 😓

[joaopgrassi]

I see we have these metrics

v8js.memory.total
v8js.memory.used

v8js.heap.total
v8js.heap.used

That are basically duplicated. Did you consider grouping them in a single v8js.memory.total|used, and then having a v8js.memory.type = heap|non_heap like the JVM one has? https://github.com/open-telemetry/semantic-conventions/blob/main/docs/runtime/jvm-metrics.md#metric-jvmmemoryused

Then you are down to 2 metrics instead of 4.

[joaopgrassi]

I also see we use veightjs and v8js in id and metric name. It would be good to be consistent.. and probably best to avoid the number in the metric name. I'd suggest to use veightjs everywhere.

[maryliag]

I also see we use veightjs and v8js in id and metric name. It would be good to be consistent.. and probably best to avoid the number in the metric name. I'd suggest to use veightjs everywhere.

I only used the veight version on the ids used to generate the readme, they don't show up in the docs for the final user. I only used because there was a limitation on the docs generation that was not working properly when there were numbers on the ID name. Once a new version of the plugin with the fix is added, this can be updated to use v8. I prefer keeping v8 on the metric name, and not force a limitation of our tooling on the final result to the user. What do you think?

In case you're curios, the PR with the fix: open-telemetry/weaver#152

[legendecas]

I'd prefer keeping v8 or V8 since it is the name of the engine.

Once a new version of the plugin with the fix is added, this can be updated to use v8

Sounds good to me

[joaopgrassi]

@maryliag can you please file an issue so we don't forget to change the ids to use v8js?

[joaopgrassi]

Thanks for the links @maryliag those were helpful for non JS experts like me :D. I think this looks pretty good. I think it would be a good idea to include them in the docs somewhere. Either in each metric or at least in the initial parts of the markdown.

Also: Do you think having some sort of direction on how these metrics are collected makes sense?

One thing I'm thinking is: there's several metrics in the "space" namespace and I wonder if we shouldn't convert that to an actual namespace. So like v8js.heap.space_used_size becomes v8js.heap.space.used_size and etc.

What do you think?

[maryliag]

can you please file an issue so we don't forget to change the ids to use v8js?

@joaopgrassi created the issue #1173

[maryliag]

Included notes with the links for the v8 functions, so it's more clear where they come from and their names.
Updated the space names, to have space.* instead of space_*

Do you think having some sort of direction on how these metrics are collected makes sense?

Not sure what else could be added for guidance. Someone else has actually started to implement these metrics on open-telemetry/opentelemetry-js-contrib#2136, so for someone who knows this area, should be more straight forward.

[lmolkova]

@maryliag curious about CPU - is the plan to report process.cpu.time or follow up with JS-specific CPU metric?

[maryliag]

curious about CPU - is the plan to report process.cpu.time

yes, the plan is to use process.cpu.time