fix(jinad): ensure log is ready on creating flow #2279

mohamed--abdel-maksoud · 2021-04-03T16:53:40Z

A minimal fix for #1812: right after creating a flow, the function keeps checking for the flow's log file (up to 1 minute).

If it is possible to have a tighter integration with fluentd, a better fix could be implemented.

codecov · 2021-04-03T16:57:43Z

Codecov Report

Merging #2279 (92143ff) into master (c8fb6b6) will decrease coverage by 16.32%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #2279       +/-   ##
===========================================
- Coverage   90.42%   74.09%   -16.33%     
===========================================
  Files         211      192       -19     
  Lines       11444    10889      -555     
===========================================
- Hits        10348     8068     -2280     
- Misses       1096     2821     +1725

Flag	Coverage Δ
daemon	`49.71% <ø> (-1.40%)`	⬇️
jina	`73.92% <ø> (-16.95%)`	⬇️

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

Impacted Files	Coverage Δ
jina/types/request/common.py	`0.00% <0.00%> (-100.00%)`	⬇️
jina/jaml/parsers/default/v1.py	`0.00% <0.00%> (-100.00%)`	⬇️
jina/types/ndarray/sparse/numpy.py	`0.00% <0.00%> (-100.00%)`	⬇️
jina/types/ndarray/sparse/pytorch.py	`0.00% <0.00%> (-100.00%)`	⬇️
jina/types/ndarray/sparse/tensorflow.py	`0.00% <0.00%> (-100.00%)`	⬇️
jina/optimizers/flow_runner.py	`0.00% <0.00%> (-97.78%)`	⬇️
jina/optimizers/parameters.py	`0.00% <0.00%> (-97.50%)`	⬇️
jina/optimizers/__init__.py	`0.00% <0.00%> (-97.15%)`	⬇️
jina/optimizers/discovery.py	`0.00% <0.00%> (-80.27%)`	⬇️
jina/drivers/debug.py	`23.33% <0.00%> (-73.34%)`	⬇️
... and 98 more

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 387939b...92143ff. Read the comment docs.

github-actions · 2021-04-03T17:09:04Z

Latency summary

Current PR yields:

😶 index QPS at 904, delta to last 3 avg.: +0%
😶 query QPS at 13, delta to last 3 avg.: -2%

Breakdown

Version	Index QPS	Query QPS
current	904	13
`1.1.1`	903	13

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

deepankarm

Thanks for the awesome PR @mohamed--abdel-maksoud.

Few points:

The adapter of jinad + fluentd to stream logs (jina dashboard) already takes care of Right after starting a flow, GET logs sends 404 #1812 on the frontend code.
jinad supports flow creation even when fluentd is not installed, hence we cannot rely on a local folder creation to send back flow_id.
Current integration of fluentd with jinad is very simple as it is local file-based and we plan to move it to a more optimal solution soon.

About the changes to the logstream endpoint, can you briefly explain how it would help us if we move away from WebSocketEndpoint?

mohamed--abdel-maksoud · 2021-04-06T07:37:43Z

Thanks for the feedback @deepankarm

The adapter of jinad + fluentd to stream logs (jina dashboard) already takes care of #1812 on the frontend code.

Sorry, I didn't notice that part, so it is solved in dashboard?

jinad supports flow creation even when fluentd is not installed, hence we cannot rely on a local folder creation to send back flow_id.

True, there should've been a condition on jinargs.no_fluentd

how it would help us if we move away from WebSocketEndpoint?

beside being a more idiomatic way to do websockets in FastAPI, it allows us to pass query parameters (e.g. timeout). Which in turn allows unit-testing that endpoint.
A question I had while working on this is why this part was implemented as websocket? The connection is unidirectional and can be done by server-sent events. This would solve the missing documentation issue for this endpoint.

deepankarm · 2021-04-06T07:53:13Z

A question I had while working on this is why this part was implemented as websocket? The connection is unidirectional and can be done by server-sent events. This would solve the missing documentation issue for this endpoint.

Good question. The initial implementation had bi-directional communication, hence the implementation was using websockets (and WebSocketEndpoint because of some additional logic). Ideally, we can move away to SSE, but that would need an extension library with starlette.

Since we are looking at an optimal place to store & stream fluentd logs anyway in near future, websocket based stream might be optimized/removed. I would definitely accept the PR if we only keep the websocket endpoint refactoring and remove the section with wait until _log_is_ready.

deepankarm · 2021-04-06T07:56:37Z

Also, please rebase your branch with master to make sure CI runs.

mohamed--abdel-maksoud · 2021-04-06T08:39:32Z

I would definitely accept the PR if we only keep the websocket endpoint refactoring and remove the section with wait until _log_is_ready
Also, please rebase your branch with master to make sure CI runs.

OK, I will update the PR this evening. Thanks for the review!

JoanFM · 2021-04-06T16:39:20Z

daemon/api/endpoints/logs.py

+                await self.disconnect(connection)
+
+
+@router.websocket("/logstream/{workspace_id}/{log_id}")


Hey @mohamed--abdel-maksoud , could you please use single quote?

daemon/api/endpoints/logs.py

mohamed--abdel-maksoud · 2021-04-06T17:17:21Z

@deepankarm I did the requested changes, I also pulled latest master and rebased my branch, it says it is up to date, so I'm not sure what's missing in branch to pass CI.
Also, since the score of the PR changed, I'm also not sure the branch name should stay the same.

mohamed--abdel-maksoud · 2021-04-06T17:46:55Z

@deepankarm I did the requested changes, I also pulled latest master and rebased my branch, it says it is up to date, so I'm not sure what's missing in branch to pass CI.

I think it's related to this issue: jitterbit/get-changed-files#4

deepankarm · 2021-04-07T04:41:04Z

I think it's related to this issue: jitterbit/get-changed-files#4

@cristianmtr can you help here?

nan-wang · 2021-04-08T07:54:58Z

daemon/api/endpoints/flow.py

@@ -1,3 +1,4 @@
+import asyncio


What is the motivation of adding this import?

My bad, there used to be asyncio.sleep but I removed it.
Strange the linter didn't catch it!

cristianmtr · 2021-04-08T08:56:23Z

@deepankarm I did the requested changes, I also pulled latest master and rebased my branch, it says it is up to date, so I'm not sure what's missing in branch to pass CI.

Make sure you pull the master from jina-ai/jina, not your own fork. Did you do that?

…4-1812

mohamed--abdel-maksoud · 2021-04-08T17:47:43Z

@deepankarm I did the requested changes, I also pulled latest master and rebased my branch, it says it is up to date, so I'm not sure what's missing in branch to pass CI.

Make sure you pull the master from jina-ai/jina, not your own fork. Did you do that?

Thanks, that was it. I totally forgot to sync with upstream.

deepankarm · 2021-04-09T06:42:16Z

Nice work @mohamed--abdel-maksoud

nan-wang

LGTM👍

fix(jinad): ensure log is ready on creating flow

b5a9e06

mohamed--abdel-maksoud requested a review from a team as a code owner April 3, 2021 16:53

mohamed--abdel-maksoud requested review from davidbp and Roshanjossey April 3, 2021 16:53

jina-bot added size/S area/daemon labels Apr 3, 2021

mohamed--abdel-maksoud added 6 commits April 4, 2021 20:02

fix(jinad): implement logstream websocket in more idomatic way

212a5da

fix(jinad): simplify logstream and prevent duplicate messages

1a2ebf6

feat(jinad): add timeout parameter to logstream

571fb46

fix(jinad): apply timeout to waiting for logfile too

58d10a7

fix(jinad): re-user _log_is_ready

4f28965

test(jinad): cover logs endpoints with tests

b776ded

jina-bot added size/M area/testing This issue/PR affects testing and removed size/S labels Apr 4, 2021

bwanglzu requested review from deepankarm and removed request for davidbp and Roshanjossey April 6, 2021 06:05

deepankarm requested changes Apr 6, 2021

View reviewed changes

deepankarm mentioned this pull request Apr 6, 2021

Right after starting a flow, GET logs sends 404 #1812

Closed

JoanFM reviewed Apr 6, 2021

View reviewed changes

fix: undo waiting for log

881a06c

mohamed--abdel-maksoud force-pushed the fix-jinad-flow-404-1812 branch from b4f2b0a to 881a06c Compare April 6, 2021 17:11

fix: missing dep & undefined name

91ca285

nan-wang requested changes Apr 8, 2021

View reviewed changes

mohamed--abdel-maksoud added 2 commits April 8, 2021 19:38

fix: remove unneeded import

50b6b17

Merge remote-tracking branch 'upstream/master' into fix-jinad-flow-40…

8a8bf83

…4-1812

mohamed--abdel-maksoud added 3 commits April 8, 2021 20:08

fix: add dockstrings to ConnectionMAnager class

456b21d

fix: add parameters to docstrings

abbb7e5

fix: docstrings

92143ff

deepankarm approved these changes Apr 9, 2021

View reviewed changes

deepankarm requested a review from nan-wang April 9, 2021 06:42

nan-wang approved these changes Apr 12, 2021

View reviewed changes

nan-wang merged commit 2df2af5 into jina-ai:master Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jinad): ensure log is ready on creating flow #2279

fix(jinad): ensure log is ready on creating flow #2279

mohamed--abdel-maksoud commented Apr 3, 2021

codecov bot commented Apr 3, 2021 •

edited

github-actions bot commented Apr 3, 2021 •

edited

deepankarm left a comment

mohamed--abdel-maksoud commented Apr 6, 2021

deepankarm commented Apr 6, 2021

deepankarm commented Apr 6, 2021

mohamed--abdel-maksoud commented Apr 6, 2021

JoanFM Apr 6, 2021

mohamed--abdel-maksoud commented Apr 6, 2021

mohamed--abdel-maksoud commented Apr 6, 2021

deepankarm commented Apr 7, 2021

nan-wang Apr 8, 2021

mohamed--abdel-maksoud Apr 8, 2021

cristianmtr commented Apr 8, 2021

mohamed--abdel-maksoud commented Apr 8, 2021

deepankarm commented Apr 9, 2021

nan-wang left a comment

		await self.disconnect(connection)


		@router.websocket("/logstream/{workspace_id}/{log_id}")

fix(jinad): ensure log is ready on creating flow #2279

fix(jinad): ensure log is ready on creating flow #2279

Conversation

mohamed--abdel-maksoud commented Apr 3, 2021

codecov bot commented Apr 3, 2021 • edited

Codecov Report

github-actions bot commented Apr 3, 2021 • edited

Latency summary

Breakdown

deepankarm left a comment

Choose a reason for hiding this comment

mohamed--abdel-maksoud commented Apr 6, 2021

deepankarm commented Apr 6, 2021

deepankarm commented Apr 6, 2021

mohamed--abdel-maksoud commented Apr 6, 2021

JoanFM Apr 6, 2021

Choose a reason for hiding this comment

mohamed--abdel-maksoud commented Apr 6, 2021

mohamed--abdel-maksoud commented Apr 6, 2021

deepankarm commented Apr 7, 2021

nan-wang Apr 8, 2021

Choose a reason for hiding this comment

mohamed--abdel-maksoud Apr 8, 2021

Choose a reason for hiding this comment

cristianmtr commented Apr 8, 2021

mohamed--abdel-maksoud commented Apr 8, 2021

deepankarm commented Apr 9, 2021

nan-wang left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2021 •

edited

github-actions bot commented Apr 3, 2021 •

edited