This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add metrics to track
/messages
response time by room size #13545Add metrics to track
/messages
response time by room size #13545Changes from 13 commits
64d9900
e8e4af2
f106a64
5f2421d
2e06f75
2ca4684
018c933
0ea47ac
355aacb
6c3f5e1
45207fe
5ae4cd6
d007956
36cdce4
00124a2
b1e7a76
4a1894d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I could put the
@trace
on the outside but the cached stuff does not like getting wrapped. Same with the other caching decorators I've noticed (like@cachedList
). It returns aDeferredCacheDescriptor
/DeferredCacheListDescriptor
instead of a wrapped function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make
DeferredCacheDescriptor
have a__name__
field to make it compatible.That said, is there any point in tracing the cached function from the outside? If it's a cache hit then it should take negligible time and it's probably not that interesting in your trace view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was exploring that option a few days ago but there is more to change to make it work.
It's nice to have to so you know whether it's running. It's weird to just not see a call in the trace when you see it in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this
@trace
call is messing up some of the tests (passes without it). The method still gives results so it's unclear to me why it would change things 🤔https://github.com/matrix-org/synapse/runs/7891614050?check_suite_focus=true#step:7:3384
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if it's because the function being returned by
trace
always has the same__name__
and so just a guess, but these two get the same cache entries. Seems like a massive trap. I'll see if I can confirm.edit: nope, the
__name__
passes through@trace
like you'd hope, so it's not that. It would be good to know why this is causing a test to fail :/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll re-introduce this in another PR if I get an inkling to have it again ⏩