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

React UI: Implement missing TSDB head stats section #7876

Merged
merged 2 commits into from Sep 29, 2020

Conversation

hooten
Copy link
Contributor

@hooten hooten commented Sep 1, 2020

This PR implements the missing TSDB head stats section in the React UI.

It exposes the following fields in the /api/v1/status/tsdb to make them available in the UI:

  • numSeries
  • chunkCount
  • minTime
  • maxTime

It provides functionality comparable to that of #7544 (the string formatting is slightly different).

Screen Shot 2020-09-01 at 3 34 22 PM

Closes #7546

@hooten
Copy link
Contributor Author

hooten commented Sep 8, 2020

Hey @juliusv I'd appreciate a review whenever you get a chance. Thanks!

Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry, I'm a bit behind on everything, but finally got around to studying this closer!

Looks mainly great, modulo a couple of comments.

@@ -892,6 +892,10 @@ The following endpoint returns various cardinality statistics about the Promethe
```
GET /api/v1/status/tsdb
```
- **numSeries**: This provides the number of series in the tsdb.
Copy link
Member

Choose a reason for hiding this comment

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

When mentioned as a normal word, I'd say let's keep TSDB capitalized.

@@ -892,6 +892,10 @@ The following endpoint returns various cardinality statistics about the Promethe
```
GET /api/v1/status/tsdb
```
- **numSeries**: This provides the number of series in the tsdb.
- **chunkCount**: This provides the number of chunks in the tsdb.
- **minTime**: This provides the current minimum time in the tsdb.
Copy link
Member

Choose a reason for hiding this comment

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

How about mentioning the unit (milliseconds)? So "time" -> "timestamp in milliseconds".

@@ -892,6 +892,10 @@ The following endpoint returns various cardinality statistics about the Promethe
```
GET /api/v1/status/tsdb
```
- **numSeries**: This provides the number of series in the tsdb.
Copy link
Member

Choose a reason for hiding this comment

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

Since all these stats are not for the full TSDB, but just for the head block, we should clarify that in the docs here.

I'm also thinking we should make it clear in the field names themselves. Either wrap them all in a headStats: {...} object or prefix them all with head?

@@ -902,6 +906,10 @@ $ curl http://localhost:9090/api/v1/status/tsdb
{
"status": "success",
"data": {
"numSeries": 508,
"chunkCount": 937,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what to do about these two fields, as they are also reported by /api/v1/status/runtimeinfo already (see https://prometheus.demo.do.prometheus.io/api/v1/status/runtimeinfo), and exposing the same stats in two different endpoints seems like suboptimal API design.

Probably they shouldn't be part of runtimeinfo nowadays, but just part of the new tsdb stats.

How about removing them from the existing runtimeinfo stats, as the docs for that endpoint above don't even mention those fields (among other missing ones), and there's a notice saying that fields are allowed to change between Prometheus versions.

return (
<div>
<h2>TSDB Status</h2>
<h3 className="p-2">Head Stats</h3>
<Table bordered size="sm" striped>
Copy link
Member

Choose a reason for hiding this comment

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

All other tables are embedded in a className="p-2" div, causing them to have some padding around them, but this one doesn't, so it stands out more to the sides. Could you make it consistent?

@hooten
Copy link
Contributor Author

hooten commented Sep 16, 2020

Awesome! Thanks for the review! I will address these things in the next couple of days.

@hooten hooten force-pushed the tsdb-stats-ui branch 2 times, most recently from ab42fc2 to 6a73b96 Compare September 28, 2020 20:57
Signed-off-by: Dustin Hooten <dhooten@splunk.com>
@hooten
Copy link
Contributor Author

hooten commented Sep 29, 2020

@juliusv I think I was able to address all of your comments. Please let me know if there's anything else I should update. Thanks!

@hooten hooten requested a review from juliusv September 29, 2020 14:24
if *mF.Name == "prometheus_tsdb_head_chunks" {
m := *mF.Metric[0]
if m.Gauge != nil {
chunkCount = int64(m.Gauge.GetValue())
Copy link
Member

Choose a reason for hiding this comment

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

Add a break after this to discontinue the loop?

Signed-off-by: Dustin Hooten <dhooten@splunk.com>
@juliusv
Copy link
Member

juliusv commented Sep 29, 2020

👍 Looks good now, thanks again :)

@juliusv juliusv merged commit 916dbd4 into prometheus:master Sep 29, 2020
@hooten
Copy link
Contributor Author

hooten commented Sep 29, 2020

Great! Thanks for the thorough review!

@hooten hooten deleted the tsdb-stats-ui branch September 29, 2020 20:12
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.

React UI: Implement missing TSDB head stats section
2 participants