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 process.threads #2705

Merged
merged 11 commits into from
Aug 17, 2022
Merged

Add process.threads #2705

merged 11 commits into from
Aug 17, 2022

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Aug 2, 2022

Changes

Adds a new metric, process.threads, to the process metrics available.

Related issues open-telemetry/opentelemetry-collector-contrib#12482

@atoulme atoulme requested review from a team August 2, 2022 00:53
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels Aug 2, 2022
@atoulme
Copy link
Contributor Author

atoulme commented Aug 2, 2022

Changelog updated, thanks!

@atoulme atoulme changed the title Add process.threads.count Add process.threads Aug 2, 2022
@atoulme
Copy link
Contributor Author

atoulme commented Aug 8, 2022

Here is a bit I'll present tomorrow at the spec meeting:
I believe process.threads is a finite metric, and will not be used as a namespace. An other namespace, under thread.*, should be created if we ever standardize data under a thread. This is because listening to thread activity would typically be done via a different type of monitor than a monitor for processes. For threads, we would use instrumentation in-process, such as jmx, to listen to thread activity.

Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

I feel that this semantic convention belongs in the process.runtime.* section. In Go, for example, there is no "thread" concept and we already have a runtime-metrics count of goroutines.

The suffix .count is problematic because of the way it will translate into Prometheus/OpenMetrics. I would suggest that process.runtime.threads is a fine name, however--you will know it's a count because of the point kind. EDIT: this was already addressed.

@bogdandrutu
Copy link
Member

@jmacd are you proposing process.runtime.thread.count?

@reyang
Copy link
Member

reyang commented Aug 9, 2022

I feel that this semantic convention belongs in the process.runtime.* section. In Go, for example, there is no "thread" concept and we already have a runtime-metrics count of goroutines.

I think the fact that Golang and Node.js don't have thread is a good reason why this shouldn't be part of process.runtime.*. I feel threads is an operating system concept. Although there are indeed operating systems that don't have threads, it seems pretty common to park it under OS and process.

@reyang
Copy link
Member

reyang commented Aug 9, 2022

@jmacd are you proposing process.runtime.thread.count?

process.runtime.thread.count might have a different semantic.

I'll take .NET as an example, process.thread.count could mean "how many threads does this process have (from the operating system's point of view)", while process.runtime.thread.count could mean "how many managed threads (and a better name might be process.runtime.dotnet.managed_threads because it's explicit and it doesn't make much sense to maintain the same name between .NET and Java - to avoid accidental join/merge) does the .NET runtime inside this process have" (and there could be a more complex situation where multiple instances of runtime coexist, although I've never seen that in reality except for some SQL stored procedure hosting).

@atoulme
Copy link
Contributor Author

atoulme commented Aug 9, 2022

I agree with @reyang - as said on Slack:

the process.runtime namespace does not allow to set a threads entry in it, since it is supposed to process.runtime.{environment} as per spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/runtime-environment-metrics.md
Second, the number of threads is the number of threads exposed at the OS level, not the level of threads or goroutines as experienced inside the program. That's the difference say between looking at Java threads and the number of threads in top -H -p <pid>.

@atoulme
Copy link
Contributor Author

atoulme commented Aug 15, 2022

@jmacd and anyone - any insights or follow up?

@carlosalberto
Copy link
Contributor

Let's discuss it on today's call (with whoever can make it) - hopefully we can merge this (either with minor renaming or not) today ;)

@bogdandrutu
Copy link
Member

I feel that this semantic convention belongs in the process.runtime.* section. In Go, for example, there is no "thread" concept and we already have a runtime-metrics count of goroutines.

Inside the go VM indeed cannot read threads, from outside you can actually read the number of OS threads for the go process.

@atoulme
Copy link
Contributor Author

atoulme commented Aug 17, 2022

What is the next step here?

@carlosalberto
Copy link
Contributor

@jmacd Do you have a strong instance against going with process.threads? This was briefly discussed in the past Spec call, and the Golang case was deemed as a very-specific case compared to the rest of the languages, when it comes to this item, so we could deem it as a special case (Python also has coroutines, but it also has threads - that is the closest IIRC).

@carlosalberto carlosalberto merged commit 2f6ea31 into open-telemetry:main Aug 17, 2022
@atoulme atoulme deleted the patch-1 branch August 17, 2022 18:24
dashpole pushed a commit to dashpole/opentelemetry-specification that referenced this pull request Aug 18, 2022
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.