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

Show gomaxprocs on status page #1468

Closed
grobie opened this Issue Mar 8, 2016 · 20 comments

Comments

Projects
None yet
7 participants
@grobie
Copy link
Member

grobie commented Mar 8, 2016

It'd be helpful to have access to golang runtime information somewhere.

@grobie grobie added the enhancement label Mar 8, 2016

@fabxc fabxc added kind/enhancement and removed enhancement labels Apr 28, 2016

@beorn7 beorn7 added the component/ui label Nov 2, 2016

@CrazyByDefault

This comment has been minimized.

Copy link
Contributor

CrazyByDefault commented May 14, 2017

What other Go runtime information would be relevant, other than gomaxprocs?

@grobie

This comment has been minimized.

Copy link
Member Author

grobie commented May 14, 2017

GOGC I guess.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented May 14, 2017

Prometheus stats like ingestion rate, chunk counts and time series would be useful.

@serathius

This comment has been minimized.

Copy link
Contributor

serathius commented Mar 6, 2018

If no one is working on it, can i take it?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 6, 2018

Sure.

@serathius

This comment has been minimized.

Copy link
Contributor

serathius commented Mar 6, 2018

@brian-brazil What's best way to access stats like chunk and time series counts?
Some of them are available as a metric in DefaultGatherer, but to access them we would need to search them by name.
Ingestion rate would need to be calculated, but based on which metric?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 6, 2018

You'd need to add an API to get them from the TSDB. Ingestion rate will be a bit tricky, as that changes over time.

@serathius

This comment has been minimized.

Copy link
Contributor

serathius commented Mar 6, 2018

@brian-brazil Is this ok? serathius@0873cc8
To config reload and wal corruptions described #1481 I would need to add Status to WAL interface and also pass it from main.go

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 6, 2018

It feels dirty to use metrics like that.

@serathius

This comment has been minimized.

Copy link
Contributor

serathius commented Mar 6, 2018

I agree. Do you know other less hacky method?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 6, 2018

Maybe track it more directly in the TSDB?

@serathius

This comment has been minimized.

Copy link
Contributor

serathius commented Mar 7, 2018

Yes, we could track metric value in separate variable and always update it when metric is updated. But it creates second source of truth and would be hard to avoid breaking it in future.

To contain dirty part of extracting value we could use function ToFloat64 implemented like in prometheus/client_golang#58

@serathius

This comment has been minimized.

Copy link
Contributor

serathius commented Mar 7, 2018

I added other runtime information to status page using ToFloat64 here serathius@a8018eb#diff-3acf7247bfcbc5bf815ae1f421d34342R24

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 7, 2018

@beorn7 What's your opinion on using metrics as shown in the above code example for use elsewhere?

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Mar 7, 2018

It's tempting to use metrics for that. However, one of the reasons for not providing "Getters" on metric types is to avoid encouraging exactly this usage pattern.

My take so far was the following: If I have a need to track some value for use outside of metrics collection by Prometheus, I track that value completely outside of the Prometheus metrics library code. I then use https://godoc.org/github.com/prometheus/client_golang/prometheus#NewGaugeFunc or https://godoc.org/github.com/prometheus/client_golang/prometheus#NewCounterFunc to funnel that separately tracked value into the Prometheus metrics collection. Let's say (imaginary example!) I need to track the number of chunks because I want to throttle ingestion if it passes a certain threshold. I should not funnel that value through the Prometheus library. This would just be needlessly complex including a roundtrip of converting an int into a float and back. Sure, the Prometheus library conveniently wraps atomic increments and decrements, but that's really one line with the atomic package anyway. Note that this approach avoids the duplicated sources of truth mentioned in #1468 (comment) as the one source of truth is the separately tracked value, which is merely mirrored into the Prometheus universe at collection time with the GaugeFunc or CounterFunc.

The use case for this issue is perhaps slightly different as you don't need those numbers for some internal use but to display a status page, which – arguably – is a curated and nicer looking human-readable version of /metrics. However, if we really want to go down that path of considering all those status information “just metrics”, we should not pollute the interface of the tsdb.DB by adding a Status method. We should realize that tsdb.DB already has a way to expose metrics: https://godoc.org/github.com/prometheus/tsdb#Open takes a prometheus.Registerer, and that's how metrics are communicated by the tsdb.DB. That means, we can call the Gather method of the registry we have passed in as a Registerer whenever we render the status page, and then populate status.ConfigStatus from the resulting []*dto.MetricFamily slice. That will be a bit fiddly at this one code location (thanks to the clunky protobuf representation in Go), but at least you would not need to propagate code changes all the way down to the tsdb package.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 7, 2018

That makes sense. There'll be other non-TSDB metrics we want (and also calculating some rates) so working from the default registry is probably what makes sense here.

@serathius

This comment has been minimized.

Copy link
Contributor

serathius commented Mar 7, 2018

Using Gather was my first idea, but I skipped it because it's more complicated and also relies on reversing logic of construction metric name. So still it will need to have knowledge about name and namespace of metric and rely on it not changing. In the worst case something in chain changes the resulting name, and we have empty status page.
But if it does not pose a problem, we can go with it.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 7, 2018

There's no namespaces in the proto, it's the same structure and data as what you're working with via WriteTo.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Mar 7, 2018

Note that the Gather method returns MetricFamilies, while the Write writes into a dto.Metric. There is a longer history and (partially flawed) rationale behind this distinction, which would be too long to explain here. But in any case, what you get with Gather has all the names in it.

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 22, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 22, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.