feat: add successful, pending and failed metrics to HeadRuntime #5374
Conversation
JoanFM
changed the title
github-actions
bot
added
size/L
area/core
Codecov Report
@@ Coverage Diff @@
## master #5374 +/- ##
==========================================
- Coverage 86.83% 85.66% -1.18%
==========================================
Files 100 101 +1
Lines 6512 6598 +86
==========================================
- Hits 5655 5652 -3
- Misses 857 946 +89
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
JoanFM
changed the title
JoanFM
force-pushed
the
head-runtime-metrics
branch
from
6e19557 to
d910673
Compare
JoanFM
force-pushed
the
head-runtime-metrics
branch
from
2537471 to
82c048d
Compare
JoanFM
changed the title
JoanFM
changed the title
JoanFM
force-pushed
the
head-runtime-metrics
branch
from
394a361 to
a05b6a6
Compare
JoanFM
requested review from
samsja,
alaeddine-13,
girishc13,
Jackmin801 and
JohannesMessner
| ) | ||
|
|
||
| if self._receiving_request_metrics: | ||
| self._request_init_time[request.request_id] = time.time() |
There was a problem hiding this comment.
Not related to this PR but I want to mention it. This map is not always safely cleared after every request ends. Especially in the head _gather_worker_tasks is not catching all exceptions. Right now it's focused on the AioRpcError and InternalNetworkError for retrying. In case if something else happens then the the _update_end_successful_requests_metrics method will not correctly remove entries from the dict.
There was a problem hiding this comment.
let's open a ticket for this, I am not sure there can be other errors,it should be handled in another PR
| if result.status.code != jina_pb2.StatusProto.ERROR: | ||
| self._update_end_successful_requests_metrics(result) | ||
| else: | ||
| self._update_end_failed_requests_metrics() |
There was a problem hiding this comment.
Should we also pop the Request entry in this method?
There was a problem hiding this comment.
I am just moving this method around
|
📝 Docs are deployed on https://head-runtime-metrics--jina-docs.netlify.app 🎉 |
alaeddine-13
changed the title
Goals:
Some metrics should be in the Head as they are in the Gateway
resolves Catch up with the missing metrics in the Head #5184
Refactor HeaderRequestHandler inheriting MonitoringMixin
Do the proper private calls
Add tests
Update docs