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 thread state metrics #910

Merged
merged 4 commits into from Oct 12, 2018
Merged

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Oct 8, 2018

This PR adds thread state metrics.

Note that this could be enhanced if there's a way to reuse a result across gauges but I couldn't find an easy way yet. #807 might be helpful here.

Closes gh-543

@mweirauch
Copy link
Contributor

I never got around looking into this, but would have thought of 'jvm.thread.states' with 'id' tag for the state. So you'd get all states (arguably the remaining are not that interesting) and this would differentiate this metric from the 'jvm.threads.*' ones. But just thinking loud. Thanks for looking into this!

@izeye
Copy link
Contributor Author

izeye commented Oct 8, 2018

@mweirauch Thanks for the feedback! I just tried to translate the request from the referenced issue into code and didn't think much on this. I'd like to understand your suggestion but I couldn't get it fully. I might miss some context there.

I guess you're suggesting metric per thread and the meter definition is as follows:

  • Meter name: jvm.thread.states
  • Tag: id for thread ID

What I didn't get fully is how the state will be represented there. Translating it into numeric value or having another tag like state?

@mweirauch
Copy link
Contributor

What I meant was actually encoding the state as 'id' tag. I didn't consider the thread id. But what about 'id' and 'state' tags?

Or does this generate too much cardinality given the fact that every thread id can be in every state?

@izeye
Copy link
Contributor Author

izeye commented Oct 9, 2018

What I meant was actually encoding the state as 'id' tag. I didn't consider the thread id.

@mweirauch Ah, I'm just confused by the name, id with thread IDs. I have no opinion on the meter names and interpreting states into tags. @jkschneider might have an opinion on this.

But what about 'id' and 'state' tags?

Again, I'm just confused by the name, id with thread IDs, so I'm not sure it's worth to be a tag. And TBH I don't know how it could be achieved with MeterBinder for now.

@shakuzen
Copy link
Member

shakuzen commented Oct 9, 2018

I'm in favor of one meter with state tag rather than separate meters per state. The sum of all states is meaningful, so a tag seems a better fit than separate metrics, IMO.

But what about 'id' and 'state' tags?

Or does this generate too much cardinality given the fact that every thread id can be in every state?

Thread ID seems high cardinality to me and limited value. If we tag any identity at all, perhaps thread name is a better thing to tag? Thread name would likely be the same or similar across applications/instances/restarts for the same logical threads.

As for the meter name, it seems like this is actually an enhancement to the already existing jvm.threads.live gauge by adding tagging of the states (and some thread identification like thread name). Adding up all these new slices should give the same value returned for jvm.threads.live now, but unfortunately, I don't think we can make such a change to an existing metric without breaking some people. Given that, jvm.threads.states seems alright, but it is semantically the same as jvm.threads.live just sliced by state (and maybe some thread qualifier).

@mweirauch
Copy link
Contributor

I agree thread id or name might be too much, also regarding the retrieval. And yes regarding 'jvm.threads.live', would be the same just sliced up.

So a distinct metric just with all states available expressed with a tag would do is my understanding from the discussion.

@izeye
Copy link
Contributor Author

izeye commented Oct 9, 2018

@shakuzen @mweirauch Thanks for the feedback! Having state tag for thread state value sounds reasonable. I added 1d5ff43 as suggested.

@mweirauch
Copy link
Contributor

I know nitpicking, but I think all 'Thread.State' should be part of the metric. So 'NEW' and 'TERMINATED'. Otherwise summing over all states doesn't give the whole set of threads.

@izeye
Copy link
Contributor Author

izeye commented Oct 9, 2018

@mweirauch I just assumed ThreadMXBean.getAllThreadIds() won't include threads having NEW state based on its Javadoc:

Returns all live thread IDs. Some threads included in the returned array may have been terminated when this method returns.

But I didn't check more than the Javadoc, so I might be wrong. And I thought TERMINATED state is less interesting but it might be subjective.

I just added 8061ccb as suggested.

@jkschneider jkschneider added this to the 1.1.0-rc.1 milestone Oct 12, 2018
@jkschneider jkschneider added spring-boot change Change is needed in Spring Boot for this issue and removed spring-boot change Change is needed in Spring Boot for this issue labels Oct 12, 2018
@jkschneider jkschneider merged commit 9b8a180 into micrometer-metrics:master Oct 12, 2018
@izeye izeye deleted the gh-543 branch October 12, 2018 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants