Skip to content

Conversation

@mengmengy
Copy link
Contributor

Signed-off-by: Mengmeng Yang [email protected]

What this PR does:
Add few more logs to cache results file so that it's easier to figure out a request is skipped cache, cache hit, cache miss when a request split to multiple ones.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@alanprot
Copy link
Member

We dont have metrics for it already?

@mengmengy
Copy link
Contributor Author

We dont have metrics for it already?

I think it logs few counters like cortex_cache_hits, cortex_cache_fetched_keys (like below), but from these, we cannot tell which split request got cache hit or cache miss. Please advise if I missed anything.

// TYPE cortex_cache_fetched_keys counter
cortex_cache_fetched_keys{name="frontend.redis"} 3

// TYPE cortex_cache_hits counter
cortex_cache_hits{name="frontend.redis"} 1

@alvinlin123
Copy link
Contributor

I am not sure if we should do info logging on the call path that may be high TPS. May have impact on latency.

@mengmengy
Copy link
Contributor Author

I am not sure if we should do info logging on the call path that may be high TPS. May have impact on latency.

Understood, not all logs will be executed for each split request, for each split request, only 1 log is added. Add such log will be useful when we try to figure out issues like why request is being slow etc. Or do you suggest other better ways we can figure out this I'm not aware?

@alvinlin123
Copy link
Contributor

Maybe have it to be debug log instead of info, or maybe like @alanprot suggested, use metrics instead of logs.

@mengmengy
Copy link
Contributor Author

Maybe have it to be debug log instead of info, or maybe like @alanprot suggested, use metrics instead of logs.

Metrics may not suitable for this case since it will be good to know which split request get cache hit/miss... I updated log level from info to debug.

@mengmengy
Copy link
Contributor Author

@alanprot @alvinlin123 could you please take a review about the updated PR? Thanks

@alanprot alanprot merged commit cc08682 into cortexproject:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants