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

Limit number of merics in prometheus UI #5139

Merged
merged 1 commit into from Jan 31, 2019

Conversation

Projects
None yet
5 participants
@vn-ki
Copy link
Contributor

vn-ki commented Jan 27, 2019

Closes #2119
Closes #3321

Displays only first 10000 series in the UI.
If there are more than 10000 series, show a warning.

Display only the first 100 metrics in the typeahead. I chose 100 because it did not make sense to scroll beyond that in a suggestion box and can be changed in case more is required.

Make the typeahead scrollable for easy navigation of suggestions.

Ping: @simonpasquier @codesome

Signed-off-by: Vishnunarayan K I appukuttancr@gmail.com

web/ui/static/js/graph/index.js Outdated
@@ -1143,6 +1145,25 @@ function escapeHTML(string) {
});
}

function limitMetrics(result) {
var MAX_METRICS_NUM = 10;

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Jan 28, 2019

Member

Is this the right value?

I'd suggest going with 10k, which is still a reasonable number of metrics to have.

This comment has been minimized.

Copy link
@vn-ki

vn-ki Jan 28, 2019

Author Contributor

Whoops! That was a test value.

This comment has been minimized.

Copy link
@vn-ki

vn-ki Jan 28, 2019

Author Contributor

@vn-ki vn-ki force-pushed the vn-ki:metrics-ui branch Jan 28, 2019

@codesome

This comment has been minimized.

Copy link
Member

codesome commented Jan 28, 2019

Gave a quick run. The warning doesn't go away when I query something later which fetches fewer metrics.

@vn-ki

This comment has been minimized.

Copy link
Contributor Author

vn-ki commented Jan 28, 2019

@codesome Fixed.

@vn-ki vn-ki force-pushed the vn-ki:metrics-ui branch 2 times, most recently Jan 28, 2019

@codesome
Copy link
Member

codesome left a comment

LGTM with a small change. I would ideally wait for someone who has experience in this field to comment.

web/ui/static/js/graph/index.js Outdated

if ($("#max-metrics-warning")[0]) {
// If message already exists
$("#max-metrics-warning").remove();

This comment has been minimized.

Copy link
@codesome

codesome Jan 28, 2019

Member

From a UX perspective, I think this should be done when you submit a query.

This comment has been minimized.

Copy link
@vn-ki

vn-ki Jan 28, 2019

Author Contributor

Done

This comment has been minimized.

Copy link
@vn-ki

vn-ki Jan 28, 2019

Author Contributor

Please don't merge it yet. I'm refactoring it a bit.

This comment has been minimized.

Copy link
@vn-ki

vn-ki Jan 28, 2019

Author Contributor

Done

@vn-ki vn-ki force-pushed the vn-ki:metrics-ui branch 3 times, most recently Jan 28, 2019

@simonpasquier
Copy link
Member

simonpasquier left a comment

LGTM

@codesome
Copy link
Member

codesome left a comment

Nits

web/ui/static/js/graph/index.js Outdated
var MAX_METRICS_NUM = 10000;
var message = "<strong>Warning!</strong> Fetched " +
result.length + " metrics, but displaying only first " +
MAX_METRICS_NUM + " ones.";

This comment has been minimized.

Copy link
@codesome

codesome Jan 29, 2019

Member
Suggested change
MAX_METRICS_NUM + " ones.";
MAX_METRICS_NUM + ".";
web/ui/static/js/graph/index.js Outdated
@@ -803,6 +817,7 @@ Prometheus.Graph.prototype.handleConsoleResponse = function(data, textStatus) {
tBody.append("<tr><td colspan='2'><i>no data</i></td></tr>");
return;
}
data.result = self.limitMetrics(data.result);

This comment has been minimized.

Copy link
@juliusv

juliusv Jan 30, 2019

Member

Could we change the wording from "metrics" to "series" here, since "metrics" in most Prometheus contexts refers to a metric name or all series belonging to a given metric name. Same below in the error message.

web/ui/static/js/graph/index.js Outdated
@@ -286,7 +287,7 @@ Prometheus.Graph.prototype.initTypeahead = function(self) {
self.expr.typeahead({
autoSelect: false,
source: source,
items: "all",
items: 100,

This comment has been minimized.

Copy link
@juliusv

juliusv Jan 30, 2019

Member

Can we make this limit higher, like 1000? When exploring metrics, I often just type e.g. node_ in the expression input to see what exists with that prefix. But on http://demo.robustperception.io:9090/graph?g0.range_input=1h&g0.expr=count(count%20by(__name__)%20(%7B__name__%3D~%22node_.*%22%7D))&g0.tab=1, this already returns 169 different metric names. It would be possible to use the dropdown selector for some of these use cases, but that is more cumbersome.

@vn-ki vn-ki force-pushed the vn-ki:metrics-ui branch Jan 30, 2019

@vn-ki

This comment has been minimized.

Copy link
Contributor Author

vn-ki commented Jan 30, 2019

Show resolved Hide resolved web/ui/static/js/graph/index.js Outdated
Show resolved Hide resolved web/ui/static/js/graph/index.js Outdated
Limit number of merics in prometheus UI
Signed-off-by: Vishnunarayan K I <appukuttancr@gmail.com>

@vn-ki vn-ki force-pushed the vn-ki:metrics-ui branch to 523b26a Jan 30, 2019

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Jan 31, 2019

👍 thanks!

@juliusv juliusv merged commit 108b9b0 into prometheus:master Jan 31, 2019

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.