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

custom dashboard discovery - only look for discoverOn metrics #4367

Merged
merged 7 commits into from
Sep 27, 2021

Conversation

jmazzitelli
Copy link
Collaborator

fixes: #3704

This is trying to work around the problem where api.Series is being called during dashboard discovery, which could return a huge amount of data.

We want to only look for specific metric names - those metrics listed as "discoverOn" metrics in the dashboards. Those are the only ones we care about.

@jmazzitelli jmazzitelli self-assigned this Sep 17, 2021
@jmazzitelli jmazzitelli added this to In Review in Sprint 63 (v1.41) via automation Sep 17, 2021
@jmazzitelli
Copy link
Collaborator Author

Here is one implementation - it uses a set of "OR" conditions in the prom query. I am documenting this here because I might change this implementation to something different and I want a record of this way of doing it in case I want to revert back at some point in the future (perhaps we might find this is more efficient):

	// you can use "count" here instead of "sum" for possibly even more goodness
	queryString := fmt.Sprintf("sum(%v%v) by (__name__)", metricNames[0], labelQueryString)
	for i := 1; i < len(metricNames); i++ {
		queryString = fmt.Sprintf("%v OR sum(%v%v) by (__name__)", queryString, metricNames[i], labelQueryString)
	}
	results, warnings, err := in.api.Query(in.ctx, queryString, time.Now())
	if warnings != nil && len(warnings) > 0 {
		log.Warningf("GetMetricsForLabels. Prometheus Warnings: [%s]", strings.Join(warnings, ","))
	}
	if err != nil {
		return nil, errors.NewServiceUnavailable(err.Error())
	}

	// We should only get one timeseries for each metric family name. However, just in case
	// we get duplicates, store the metric name in a map and convert to an array to remove duplicates.

	namesMap := make(map[string]bool)
	for _, item := range results.(model.Vector) {
		namesMap[string(item.Metric["__name__"])] = true
	}
	names := make([]string, 0, 5)
	for n := range namesMap {
		names = append(names, n)
	}

	return names, nil

…ooking for, and loop over the results) - this will allow us to create a smaller map (most likely the results are going to be much larger than the list of metrics we are looking for)
… we released this out in the wild and we need to know if this is a bottleneck
@jmazzitelli jmazzitelli marked this pull request as ready for review September 21, 2021 21:10
@jmazzitelli
Copy link
Collaborator Author

This is ready enough for people to review.

Copy link
Collaborator

@jshaughn jshaughn left a comment

Choose a reason for hiding this comment

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

LGTM with a few non-blocking comments

Comment on lines +307 to +309
for i := 0; i < len(metricNames); i++ {
metricsWeAreLookingFor[metricNames[i]] = true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine but maybe nicer as:

for _, m := range metricNames {
    metricsWeAreLookingFor[m] = true

prometheus/client.go Show resolved Hide resolved
@jmazzitelli
Copy link
Collaborator Author

I built and published a test image based on this PR - if someone wants to test it - use this image: quay.io/jmazzitelli/kiali:pr4367

@lucasponce
Copy link
Contributor

I see comments on the code, but I don't see screenshots testing the feature.

@jmazzitelli
Copy link
Collaborator Author

but I don't see screenshots testing the feature.

There are no screenshots to show because nothing will look different than how things look today. This just performs a different query that I hope makes things faster. But the UI will look identical to how it looks today.

@lucasponce
Copy link
Contributor

There are no screenshots to show because nothing will look different than how things look today. This just performs a different query that I hope makes things faster. But the UI will look identical to how it looks today.

Have this work being tested against the https://github.com/kiali/demos/tree/master/runtimes-demo ?

@jmazzitelli
Copy link
Collaborator Author

jmazzitelli commented Sep 23, 2021

Have this work being tested against the https://github.com/kiali/demos/tree/master/runtimes-demo ?

Built and deployed the server with this PR:

commit 0f39e20 (HEAD -> dashboard-discovery-3704, jmazzitelli/dashboard-discovery-3704)

about-box

and installed the runtimes-demo:

graph

Here's all the applications and workloads - you can see all the dashboards are discovered correctly:
w-vertx
w-springboot
w-quarkus
w-nodejs
a-vertx
a-springboot
a-quarkus
a-nodejs

@jmazzitelli jmazzitelli merged commit 5c8d211 into kiali:master Sep 27, 2021
Sprint 63 (v1.41) automation moved this from In Review to Done Sep 27, 2021
@ghost ghost added this to the v1.41.0 milestone Sep 27, 2021
@jmazzitelli jmazzitelli deleted the dashboard-discovery-3704 branch September 27, 2021 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Improve discovery matcher process for Custom Dashboards
3 participants