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 semantic conventions for jvm cpu metrics #2292

Merged
merged 18 commits into from
May 27, 2022

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented Jan 25, 2022

Initial draft for JVM runtime cpu semantic conventions.
I created a gist where you can see where these values are coming from: https://gist.github.com/jonatan-ivanov/0449619cb6f851ce67d70e47a6a1fb73

/cc @trask @jack-berg

@jonatan-ivanov jonatan-ivanov requested review from a team January 25, 2022 23:27
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 25, 2022

CLA Signed

The committers are authorized under a signed CLA.

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels Jan 26, 2022
# Conflicts:
#	specification/metrics/semantic_conventions/runtime-environment-metrics.md
@jack-berg
Copy link
Member

Here's a summary of the proposed metrics and their corresponding sources. Hopefully this makes it a bit easier to evaluate the value of these. This summarizes the information from @jonatan-ivanov's gist.

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Feb 25, 2022
@jack-berg jack-berg removed the Stale label Feb 25, 2022
@jack-berg
Copy link
Member

We discussed in the 2/24/2022 Java SIG meeting. Can java folks take a look at this and comment?

/cc @kittylyst @trask

My thoughts:

  • process.runtime.jvm.cpu.time, process.runtime.jvm.cpu.utilization are obvious and we should merge those quickly.
  • process.runtime.jvm.common.pool.parallelism seems more related to the thread pool topic we discussed. Maybe we should save it for a future PR where we declare the conventions for thread pools.
  • process.runtime.jvm.system.cpu.utilization has overlap with the more general system.cpu.utilization, but we don't have access to the data required for the attributes. I think we should include this now.
  • process.runtime.jvm.system.cpu.count I think we should incude this now.
  • process.runtime.jvm.system.cpu.load.average.1m useful, but overlaps with process.runtime.jvm.cpu.time & process.runtime.jvm.cpu.utilization. I'd be fine skipping this for now.

@trask
Copy link
Member

trask commented Feb 28, 2022

I'm thinking now that it may be good to reuse system.cpu.time, process.cpu.time, and system.cpu.count since there aren't any JVM-specific attributes that we want to capture on them.

I have sent #2388 to clarify that these should be interpreted as container-level and not host-level metrics when emitted from inside a container.

Should we scale back this PR for now to just these two?

  • process.runtime.jvm.cpu.utilization
  • process.runtime.jvm.system.cpu.utilization

I noticed there's already a system.cpu.utilization metric (and so seems likely to be a process.cpu.utilization metric), but this is defined in a way that I think is incompatible with what the JVM gives us:

system.cpu.utilization is defined as the difference in system.cpu.time measurements divided by the elapsed time

so I don't think we should reuse in this case and need one under process.runtime.jvm.*

@trask
Copy link
Member

trask commented Mar 2, 2022

I think we have more clarity now from discussions in #2384 and #2388.

I think there's still one open question #2392 (comment) and then I think we'll have clarity on how to proceed.

@jonatan-ivanov thanks for your patience

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Mar 12, 2022
@trask
Copy link
Member

trask commented Mar 12, 2022

/unstale

@github-actions github-actions bot removed the Stale label Mar 13, 2022
@jonatan-ivanov jonatan-ivanov requested a review from trask March 17, 2022 16:13
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
# Conflicts:
#	specification/metrics/semantic_conventions/runtime-environment-metrics.md
@trask
Copy link
Member

trask commented May 9, 2022

@open-telemetry/technical-committee is this not the right place to define JVM metrics that we want to emit? would it be better if we just document these in the Java repo and create our own "jvm metrics" schema url? thx

@github-actions github-actions bot removed the Stale label May 10, 2022
@bogdandrutu bogdandrutu dismissed their stale review May 10, 2022 16:36

The reason to block was resolved, still a very confusion proposal for system under process

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label May 24, 2022
@reyang reyang removed the Stale label May 24, 2022
@reyang
Copy link
Member

reyang commented May 24, 2022

I've removed the Stale tag.
Given we've got a good number of approvals from Java experts, I want to see if there is any blocker from @bogdandrutu and @jsuereth. If there is no blocker, I think we should move forward.

@reyang
Copy link
Member

reyang commented May 26, 2022

@jonatan-ivanov would you resolve the merge conflicts and clear the CI? Thanks!

# Conflicts:
#	specification/metrics/semantic_conventions/runtime-environment-metrics.md
@jonatan-ivanov
Copy link
Member Author

@reyang Would you approve the workflow to run?

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@reyang reyang merged commit b13c164 into open-telemetry:main May 27, 2022
@reyang
Copy link
Member

reyang commented May 27, 2022

@jonatan-ivanov please send a follow up PR to update the changelog (which is an editorial/trivial change).

@jonatan-ivanov
Copy link
Member Author

@reyang Thanks! Here is the PR for the changelog entry: #2584

beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants