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 metrics to monitor the query costs #51

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

guerremdq
Copy link
Contributor

@guerremdq guerremdq commented Oct 30, 2023

Expose metrics for each query to monitor the query cost


This change is Reviewable

@stephen-soltesz
Copy link
Contributor

stephen-soltesz commented Nov 1, 2023

@guerremdq -- adding these metrics would be welcomed. Thank you! Please be mindful of unit tests, code coverage, and the build status.

@guerremdq
Copy link
Contributor Author

Hey @stephen-soltesz , Need to implement a few more methods in the cloudtest library to be able to test this
m-lab/go#174

@stephen-soltesz
Copy link
Contributor

Changes from m-lab/go#174 are available in https://github.com/m-lab/go/releases/tag/v0.1.67

guerremdq and others added 3 commits November 8, 2023 10:48
* - Add metrics for sucessful and failed queries
- Add keepAlive flag to not kill the app when the query fail

* - Add metric names prefix
- Remove duplicated matric calls
- Add extra label to update duration metic
@guerremdq
Copy link
Contributor Author

Thanks @stephen-soltesz , I let me know what you think about this PR

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Thank you @guerremdq -- I've added a few suggestions that I think will make the change a little simpler.

Reviewed 1 of 7 files at r1, 1 of 7 files at r3, 1 of 1 files at r5, 2 of 2 files at r7.
Reviewable status: 0 of 1 LGTMs obtained


query/bigquery_runner.go line 22 at r7 (raw file):

}

func (b *bigQueryImpl) Query(query string, visit func(row map[string]bigquery.Value) error) (*bigquery.QueryStatistics, error) {

Rather than returning bigquery.QueryStatistics here (and for all existing cases calling Query), consider extending the BQRunner interface instead.

Then the bigQueryImpl struct could include a bigquery.QueryStatistics field that's set after a successful query and could be read by callers that need it from a new method, say, LastStats() or similar.

Then the *Collector.Update() loop would call Query() followed by LastStats() to update the metrics -- and all the return type changes throughout this PR are unnecessary.


query/bigquery_runner.go line 31 at r7 (raw file):

	}

	if job == nil {

It should not be possible for Query.Run to return a nil err and a nil job. Please remove this check.


query/bigquery_runner.go line 38 at r7 (raw file):

	status, err := job.Wait(context.Background())
	if err != nil {
		return nil, status.Err()

status is not guaranteed to be non-nil when err != nil. Prefer simply returning err here.


query/bigquery_runner.go line 44 at r7 (raw file):

	it, err := job.Read(context.Background())

nit: please remove extra new line between job.Read and the err check.


sql/collector.go line 155 at r7 (raw file):

func setSlotMillis(ch chan<- prometheus.Metric, slotMillis int64, metricName string) {
	desc := prometheus.NewDesc("bqx_slot_seconds_utilized", "slot milliseconds utilized", []string{"filename"}, nil)

If these metrics were part of the Collector they would need to be registered in Describe, iirc.

Each set of Collector metrics may be distinct from one another. These metrics are constant for the package. So, I don't think these should be part of the Collector. These are package level metrics collected about the different queries run.

Please redefine these at the package level as standard Gauges.


.travis.yml line 14 at r6 (raw file):

- make
- go test -short -v ./... -cover=1 -coverprofile=_c.cov
- $GOPATH/bin/goveralls -service=travis-ci -coverprofile=_c.cov

If you'd be willing to change "travis-ci" to "travis-pro" we may be able to get coverage stats for this PR. (But it might not work until a new PR is created)

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.

2 participants