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

Better trace op names for querier #1006

Merged
merged 1 commit into from
Oct 10, 2018

Conversation

gouthamve
Copy link
Contributor

@gouthamve gouthamve commented Sep 14, 2018

DON'T MERGE, depends on weaveworks/common#126

It separates the /metrics calls from queries now.

@bboreham
Copy link
Contributor

What's the justification for this change? The different strings don't seem that much better that it warrants 47 extra lines of code.

@gouthamve
Copy link
Contributor Author

Well having /metrics interspersed with query makes it hard to pick the right trace, while that in itself is not that big of an issue, we cannot control the sampling for both separately. We currently sample all our queries and that get includes a ton of /metrics calls.

@tomwilkie
Copy link
Contributor

I wonder if we can use https://github.com/weaveworks/common/blob/master/middleware/instrument.go#L66 to correct label the traces, as we do with the labels in the histograms?

@bboreham
Copy link
Contributor

Maybe a screen-grab would help? I can't recall ever having a problem with metrics calls interspersed.

@gouthamve gouthamve force-pushed the better-trace-operations branch from 2bd5148 to 1d720f3 Compare September 14, 2018 13:56
@gouthamve
Copy link
Contributor Author

screen shot 2018-09-14 at 19 16 53

This is the same for querier and it's the same op for queries. Now I look for other operations when looking for queries, but I'd like to be able to not sample these.

Also, I implemented Tom's suggestion and the code is much smaller now :) Like 10-ish lines :P

@gouthamve gouthamve force-pushed the better-trace-operations branch from 1d720f3 to 5485c3f Compare October 3, 2018 11:16
@gouthamve
Copy link
Contributor Author

Updated the vendoring to use upstream. Should just work now. Good to take a look.

@gouthamve gouthamve changed the title [WIP]: Better trace op names for querier Better trace op names for querier Oct 3, 2018
@tomwilkie
Copy link
Contributor

LGTM - lets test in dev first before merging though.

@tomwilkie
Copy link
Contributor

Works spot on:

screen shot 2018-10-10 at 10 36 03

@tomwilkie tomwilkie merged commit 9f6ad2b into cortexproject:master Oct 10, 2018
@tomwilkie tomwilkie deleted the better-trace-operations branch October 10, 2018 09:36
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