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

feat: add a decoratator for monitoring method #4737

Merged
merged 12 commits into from
May 9, 2022
Merged

Conversation

samsja
Copy link
Contributor

@samsja samsja commented May 2, 2022

Follow up of jina-ai/clip-as-service#674

Goal:

Having an decorator to monitor internal method of an Executor

class DummyExecutor(Executor):
    @requests(on='/foo')
    def foo(self, docs, **kwargs):
        self._proces(docs)
        self.proces_2(docs)

    @monitor(name='metrics_name',  documentation='metrics description')
    def _proces(self, docs):
        ...

    @monitor()
    def proces_2(self, docs):
        ...

Screenshot from 2022-05-02 14-44-11

@samsja samsja requested a review from hanxiao as a code owner May 2, 2022 12:40
@github-actions github-actions bot added size/M area/core This issue/PR affects the core codebase area/entrypoint This issue/PR affects the entrypoint codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing component/resource labels May 2, 2022
@codecov
Copy link

codecov bot commented May 2, 2022

Codecov Report

Merging #4737 (112e1b7) into master (f9f8600) will decrease coverage by 0.08%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #4737      +/-   ##
==========================================
- Coverage   87.73%   87.65%   -0.09%     
==========================================
  Files         119      119              
  Lines        8661     8690      +29     
==========================================
+ Hits         7599     7617      +18     
- Misses       1062     1073      +11     
Flag Coverage Δ
jina 87.65% <83.33%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/serve/executors/__init__.py 86.36% <72.72%> (-0.39%) ⬇️
jina/serve/executors/decorators.py 97.22% <91.66%> (-1.12%) ⬇️
jina/__init__.py 65.88% <100.00%> (ø)
...deployments/config/k8slib/kubernetes_deployment.py 52.08% <0.00%> (-6.25%) ⬇️
jina/serve/runtimes/gateway/http/__init__.py 97.77% <0.00%> (-2.23%) ⬇️
...a/orchestrate/deployments/config/docker_compose.py 99.00% <0.00%> (-1.00%) ⬇️
jina/orchestrate/flow/base.py 89.57% <0.00%> (-0.61%) ⬇️
jina/helper.py 81.72% <0.00%> (-0.06%) ⬇️
jina/serve/networking.py 88.02% <0.00%> (+1.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9f8600...112e1b7. Read the comment docs.

@github-actions
Copy link

github-actions bot commented May 2, 2022

Latency summary

Current PR yields:

  • 🐢🐢 index QPS at 1073, delta to last 2 avg.: -18%
  • 🐢🐢 query QPS at 53, delta to last 2 avg.: -30%
  • 🐎🐎🐎🐎 avg flow time within 2.0012 seconds, delta to last 2 avg.: +33%
  • 🐎🐎🐎🐎 import jina within 0.6085 seconds, delta to last 2 avg.: +35%

Breakdown

Version Index QPS Query QPS Avg Flow Time (s) Import Time (s)
current 1073 53 2.0012 0.6085
3.3.25 1304 79 1.6657 0.4366
3.3.24 1341 73 1.342 0.46

Backed by latency-tracking. Further commits will update this comment.

@samsja samsja force-pushed the feat-monitor-executor branch 2 times, most recently from 9b2689d to 58ba316 Compare May 2, 2022 12:54
@samsja samsja linked an issue May 2, 2022 that may be closed by this pull request
@JoanFM JoanFM marked this pull request as draft May 2, 2022 13:10
jina/serve/executors/__init__.py Outdated Show resolved Hide resolved
@samsja samsja force-pushed the feat-monitor-executor branch from 03d997a to 38c7821 Compare May 4, 2022 09:32
@samsja samsja force-pushed the feat-monitor-executor branch from 38c7821 to 314f359 Compare May 4, 2022 09:53
@samsja samsja marked this pull request as ready for review May 4, 2022 13:44
jina/serve/executors/__init__.py Outdated Show resolved Hide resolved
jina/serve/executors/decorators.py Outdated Show resolved Hide resolved
Co-authored-by: Joan Fontanals <[email protected]>
@hanxiao
Copy link
Member

hanxiao commented May 5, 2022

@monitor('metrics_name', 'metrics description')
  1. what are the available matric names?
  2. it is better to force the explicit kwargs, with *: https://stackoverflow.com/a/14298976
  3. if metrics_name is basically an enum, then why the description is required, isn't the description best described by us?

@samsja
Copy link
Contributor Author

samsja commented May 5, 2022

what are the available matric names?
it is better to force the explicit kwargs, with *: https://stackoverflow.com/a/14298976
if metrics_name is basically an enum, then why the description is required, isn't the description best described by us?

'metric_name' is the name that will appear on prometheus. For example if you are monitoring the time that clip as a service spend processing some text you will call it text_preprocessing. It is not an enum for the type of the metric, therefore the description is used to give more context on what the function which is wrapped by monitoring is doing.
Btw the user can as well just call @monitor and it will produce a name and a description automatically based on the name of the function ( everything will be documented in the documentation pr)

@samsja
Copy link
Contributor Author

samsja commented May 5, 2022

@monitor('metrics_name', 'metrics description')
1. what are the available matric names?

2. it is better to force the explicit kwargs, with `*`: https://stackoverflow.com/a/14298976

3. if metrics_name is basically an enum, then why the description is required, isn't the description best described by us?

for point 2. you are right I will update it

@samsja samsja requested a review from JoanFM May 5, 2022 13:27
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

I am missing unit tests for the monitor decorator

@samsja
Copy link
Contributor Author

samsja commented May 6, 2022

done

@JoanFM JoanFM merged commit c1e0395 into master May 9, 2022
@JoanFM JoanFM deleted the feat-monitor-executor branch May 9, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/entrypoint This issue/PR affects the entrypoint codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing epic/monitoring size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draft the three Executor monitoring interfaces
3 participants