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

Introduce jvm.thread.daemon and jvm.thread.state attributes #297

Merged
merged 8 commits into from
Oct 23, 2023

Conversation

trask
Copy link
Member

@trask trask commented Aug 31, 2023

Fixes open-telemetry/build-tools#101
Closes open-telemetry/build-tools#314

Changes

This PR does two things:

  • Removes the general thread.daemon attribute and introduces jvm.thread.daemon instead
  • Introduces jvm.thread.state attribute and adds it to the jvm.thread.count metric

It's definitely debatable whether these two attributes should live in the jvm.thread.* or thread.* namespace.

My primary reason for proposing that they live under jvm.thread.* is that I couldn't find anything to support these being general cross-language concepts, and JVM metric stability will depend on any attributes it uses also being marked stable.

I think we could view this like garbage collection, which is something that some other languages have, but we are not prepared to try to unify under a single set of attribute definitions prior to initial stability.

Note that while ECS defines thread.name and thread.id, they do not have anything like thread.daemon or thread.state.

Merge requirement checklist

@trask trask requested review from a team August 31, 2023 22:29
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

👍

@joaopgrassi
Copy link
Member

joaopgrassi commented Sep 4, 2023

In other hand, the "isDaemon" concept can be extracted from Python and .NET, (maybe also in other langs?) so maybe we should consider actually keeping it in the existing thread namespace? In other words, what is the drawback of keeping daemon where it is today vs adding in the jvm namespace?

If we later decide to spec it and re-use, the JVM metrics will be hurt by it.

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Given Java's moves in threading models where it could be the case going forward that Java threads deviate significantly from POSIX standards, I think it makes sense to split.

Per @joaopgrassi's comments on DAEMON and python/.NET, I'm not sure we have the same meaning between them all. Would be good to do a quick comparison, but I do think isolating Java's threading model from others would be healthy at this point.

@trask
Copy link
Member Author

trask commented Oct 18, 2023

Per @joaopgrassi's comments on DAEMON and python/.NET, I'm not sure we have the same meaning between them all. Would be good to do a quick comparison, but I do think isolating Java's threading model from others would be healthy at this point.

Java "daemon threads" == .NET "background threads"

I couldn't find any other languages that have a similar concept (other than Java, Python and .NET)

I'm personally in favor of keeping jvm.thread.daemon langauge specific, and if .NET wants to introduce similar they would probably name it .background

@trask trask requested a review from a team October 18, 2023 17:17
docs/runtime/jvm-metrics.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@joaopgrassi joaopgrassi merged commit d2a5612 into open-telemetry:main Oct 23, 2023
9 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.

6 participants