Skip to content

Improve ruler tracing #720

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

Merged
merged 3 commits into from
Feb 21, 2018
Merged

Improve ruler tracing #720

merged 3 commits into from
Feb 21, 2018

Conversation

bboreham
Copy link
Contributor

Using weaveworks/common/instrument in Evaluate() gives us a tracing span which ties together all the sub-operations for that rule group.

It would be better to have a span per rule, but that is inside Prometheus.

I also changed the histogram buckets for eval duration to go up to 25 seconds, and drop the low end below 100ms.

Copy link
Contributor

@jml jml left a comment

Choose a reason for hiding this comment

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

Curious as to how this interacts with #719

@cboggs
Copy link
Contributor

cboggs commented Feb 21, 2018

At a glance it seems like it should play nicely with #719, at least functionally. Looks like we're just threading UserID through the evaluation cycle to enhance traces, which should mesh reasonably well with the rules-by-file logic.

I'll merge the two locally to see how it turns out and report back shortly.

@cboggs
Copy link
Contributor

cboggs commented Feb 21, 2018

Some conflicts on mutually modified lines, but I don't see anything functionally breaking.
A bit of the conflict will go away when the HistogramVec commit from #719 is reverted.
If / when this PR is merged, we can resolve the conflicts pretty easily on #719 and call it good.

@bboreham bboreham merged commit 646ec88 into master Feb 21, 2018
@bboreham bboreham deleted the trace-ruler branch February 21, 2018 16:43
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.

3 participants