feat: pass request-headers as metadata #6126

NarekA · 2023-12-07T15:49:13Z

This PR is attempts to implement the solution in the discussion of #6049. It adds a new parameter metadata for jina endpoints which can be used to pass in http headers or GRPC metadata.

This PR only implements the http headers, GRPC metadata will have to be done in a separate PR

Goals:

resolves Best Way to Pass Request Headers to Executor #6049
check and update documentation. See guide and ask the team.

TODO:

Add tests
Add documentation

NarekA · 2023-12-07T15:51:23Z

jina/constants.py

@@ -53,6 +53,7 @@
 __args_executor_func__ = {
    'docs',
    'parameters',
+    'metadata',


Do you prefer metadata or request_metadata?

request_metadata or headers? I would say headers perhaps

NarekA · 2023-12-07T15:51:37Z

jina/proto/docarray_v1/jina.proto

@@ -122,6 +122,8 @@ message DataRequestProto {
    }

    DataContentProto data = 4; // container for docs and groundtruths
+
+    google.protobuf.Struct metadata = 5; // extra kwargs that will be used in executor


Will update this comment

I do not want to add this here. We decided that we wanted to pass as grpc-metadata which is something that should not be shown in the protobuf.

Will remove this and the proto changes that resulted from it.

NarekA · 2023-12-07T15:52:22Z

jina/serve/runtimes/worker/http_fastapi_app.py


            req = DataRequest()
            if body.header is not None:
                req.header.request_id = body.header.request_id

            if body.parameters is not None:
                req.parameters = body.parameters
+            req.metadata = dict(request.headers or {"no_headers": "true"})


Will remove this default value

cenhao · 2023-12-07T16:36:04Z

jina/serve/runtimes/gateway/http_fastapi_app_docarrayv2.py

        @app.api_route(**app_kwargs)
-        async def post(body: input_model, response: Response):
+        async def post(body: input_model, response: Response, request: Request):


Does this means the header will only be available when gateway is enabled?

Anyway - having access to HTTP header is very important for our production application - we are an infra team supporting different businesses, and we use headers to communicate extra information like auth, tracing id, etc.

@JoanFM

It should work for standalone-fast-api deployments too. See this change.

hey @cenhao ,

The feature should be available with and without gateway.

Btw, would be cool to know about ur usecase and how u use/plan to use Jina

codecov · 2023-12-07T22:26:40Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (9eb4d09) 72.61% compared to head (2856423) 49.55%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
jina/serve/runtimes/gateway/streamer.py	0.00%	8 Missing ⚠️
jina/serve/runtimes/worker/http_fastapi_app.py	0.00%	4 Missing ⚠️
...ve/runtimes/gateway/http_fastapi_app_docarrayv2.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #6126       +/-   ##
===========================================
- Coverage   72.61%   49.55%   -23.06%     
===========================================
  Files         152      150        -2     
  Lines       14061    14027       -34     
===========================================
- Hits        10210     6951     -3259     
- Misses       3851     7076     +3225

Flag	Coverage Δ
jina	`49.55% <12.50%> (-23.06%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoanFM

I would name more headers than metadata.

JoanFM · 2023-12-11T09:03:05Z

jina/proto/docarray_v1/jina.proto

@@ -122,6 +122,8 @@ message DataRequestProto {
    }

    DataContentProto data = 4; // container for docs and groundtruths
+
+    google.protobuf.Struct metadata = 5; // extra kwargs that will be used in executor


I do not want to add this here. We decided that we wanted to pass as grpc-metadata which is something that should not be shown in the protobuf.

JoanFM · 2023-12-11T09:03:28Z

jina/constants.py

@@ -53,6 +53,7 @@
 __args_executor_func__ = {
    'docs',
    'parameters',
+    'metadata',


request_metadata or headers? I would say headers perhaps

JoanFM · 2023-12-11T09:03:40Z

jina/__init__.py

@@ -81,7 +81,7 @@ def _ignore_google_warnings():

 # do not change this line manually
 # this is managed by proto/build-proto.sh and updated on every execution
-__proto_version__ = '0.1.27'
+__proto_version__ = '0.1.28'


related to the proto comment, should not be updated.

JoanFM · 2023-12-11T09:03:53Z

jina/proto/docarray_v1/jina.proto

@@ -138,6 +140,8 @@ message SingleDocumentRequestProto {

    docarray.DocumentProto document = 4; // the document in this request

+    google.protobuf.Struct metadata = 5; // extra kwargs that will be used in executor


JoanFM · 2023-12-11T09:03:58Z

jina/proto/docarray_v1/jina.proto

@@ -148,6 +152,8 @@ message DataRequestProtoWoData {

    repeated RouteProto routes = 3; // status info on every routes

+    google.protobuf.Struct metadata = 4; // extra kwargs that will be used in executor


JoanFM · 2023-12-11T09:05:02Z

jina/serve/runtimes/gateway/http_fastapi_app_docarrayv2.py

        @app.api_route(**app_kwargs)
-        async def post(body: input_model, response: Response):
+        async def post(body: input_model, response: Response, request: Request):


hey @cenhao ,

The feature should be available with and without gateway.

Btw, would be cool to know about ur usecase and how u use/plan to use Jina

NarekA · 2023-12-12T19:57:46Z

@JoanFM I'm happy to change the parameter to headers, but I'm afraid it's too close to header which I've noticed is used for passing request_id during orchestration. How about request_headers?

JoanFM · 2023-12-12T20:36:37Z

@JoanFM I'm happy to change the parameter to headers, but I'm afraid it's too close to header which I've noticed is used for passing request_id during orchestration. How about request_headers?

it does not matter, header is something that is quite hidden from the Executor developer. I would still use headers

NarekA commented Dec 7, 2023

View reviewed changes

cenhao reviewed Dec 7, 2023

View reviewed changes

NarekA requested a review from hanxiao as a code owner December 7, 2023 22:19

NarekA force-pushed the feat-pass-headers-as-metadata-6049 branch from 0fb78c9 to d1450b0 Compare December 7, 2023 22:21

JoanFM reviewed Dec 11, 2023

View reviewed changes

Rename metadata to headers

e0b54de

NarekA force-pushed the feat-pass-headers-as-metadata-6049 branch from 40f0c60 to e0b54de Compare December 13, 2023 13:50

NarekA added 2 commits December 13, 2023 05:50

Merge branch 'master' into feat-pass-headers-as-metadata-6049

c23bbf6

Merge branch 'master' into feat-pass-headers-as-metadata-6049

2856423

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: pass request-headers as metadata #6126

feat: pass request-headers as metadata #6126

NarekA commented Dec 7, 2023 •

edited

Loading

NarekA Dec 7, 2023

NarekA Dec 7, 2023

JoanFM Dec 11, 2023

NarekA Dec 7, 2023

JoanFM Dec 11, 2023

NarekA Dec 12, 2023

NarekA Dec 7, 2023

cenhao Dec 7, 2023

cenhao Dec 7, 2023

NarekA Dec 7, 2023 •

edited

Loading

JoanFM Dec 11, 2023

codecov bot commented Dec 7, 2023 •

edited

Loading

JoanFM left a comment

JoanFM Dec 11, 2023

JoanFM Dec 11, 2023

JoanFM Dec 11, 2023

JoanFM Dec 11, 2023

JoanFM Dec 11, 2023

JoanFM Dec 11, 2023

NarekA commented Dec 12, 2023

JoanFM commented Dec 12, 2023

		@@ -138,6 +140,8 @@ message SingleDocumentRequestProto {

		docarray.DocumentProto document = 4; // the document in this request

		google.protobuf.Struct metadata = 5; // extra kwargs that will be used in executor

		@@ -148,6 +152,8 @@ message DataRequestProtoWoData {

		repeated RouteProto routes = 3; // status info on every routes

		google.protobuf.Struct metadata = 4; // extra kwargs that will be used in executor

feat: pass request-headers as metadata #6126

Are you sure you want to change the base?

feat: pass request-headers as metadata #6126

Conversation

NarekA commented Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NarekA Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

JoanFM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NarekA commented Dec 12, 2023

JoanFM commented Dec 12, 2023

NarekA commented Dec 7, 2023 •

edited

Loading

NarekA Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading