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

fix: multi protocol gateway supports monitoring #5570

Merged
merged 3 commits into from
Jan 3, 2023

Conversation

AnneYang720
Copy link
Contributor

@AnneYang720 AnneYang720 commented Jan 3, 2023

@github-actions github-actions bot added size/S area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing labels Jan 3, 2023
@@ -21,10 +22,11 @@ def __init__(
self.gateways: List[BaseGateway] = []
for port, protocol in zip(self.ports, self.protocols):
gateway_cls = _get_gateway_class(protocol)
runtime_args = copy.deepcopy(self.runtime_args)
# ignore metrics_registry since it is not copyable
runtime_args = deepcopy_with_ignore_attrs(self.runtime_args, ['metrics_registry'])
Copy link
Member

Choose a reason for hiding this comment

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

are we sure deepcopy is needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to copy the runtime_args object, otherwise, how can different gateway objects hold the same reference of runtime_args but have different values of port and protocol ?

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #5570 (a28f96a) into master (3499016) will decrease coverage by 1.22%.
The diff coverage is 30.00%.

@@            Coverage Diff             @@
##           master    #5570      +/-   ##
==========================================
- Coverage   87.47%   86.25%   -1.23%     
==========================================
  Files         123      123              
  Lines        9839     9846       +7     
==========================================
- Hits         8607     8493     -114     
- Misses       1232     1353     +121     
Flag Coverage Δ
jina 86.25% <30.00%> (-1.23%) ⬇️

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

Impacted Files Coverage Δ
jina/serve/runtimes/gateway/composite/gateway.py 32.43% <30.00%> (-67.57%) ⬇️
jina/jaml/parsers/__init__.py 81.81% <0.00%> (-15.91%) ⬇️
...rate/deployments/config/k8slib/kubernetes_tools.py 86.53% <0.00%> (-13.47%) ⬇️
jina/orchestrate/flow/base.py 84.15% <0.00%> (-6.01%) ⬇️
...a/orchestrate/deployments/config/docker_compose.py 94.68% <0.00%> (-4.35%) ⬇️
jina/jaml/parsers/flow/v1.py 96.96% <0.00%> (-3.04%) ⬇️
jina/clients/helper.py 97.61% <0.00%> (-2.39%) ⬇️
jina/parsers/helper.py 54.06% <0.00%> (-2.33%) ⬇️
jina/jaml/helper.py 81.75% <0.00%> (-2.19%) ⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jina/helper.py Outdated Show resolved Hide resolved
@AnneYang720 AnneYang720 force-pushed the fix-multiprotocol-monitoring branch from 8e169ee to a28f96a Compare January 3, 2023 14:47
@github-actions github-actions bot removed the area/helper This issue/PR affects the helper functionality label Jan 3, 2023
@JoanFM JoanFM merged commit 88f9598 into master Jan 3, 2023
@JoanFM JoanFM deleted the fix-multiprotocol-monitoring branch January 3, 2023 15:18
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/testing This issue/PR affects testing size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multi protocol gateway does not support monitoring
3 participants