Core-supported context propagation #2750

ctiller · 2015-07-31T23:18:45Z

@nathanielmanistaatgoogle @murgatroid99 @soltanmm @tbetbetbe @jcanizales @jtattermusch @a-veitch after some discussion today with wrapped language folks we thought we'd try implementing context propagation directly in core.

Fixes #2747

jtattermusch · 2015-07-31T23:48:49Z

include/grpc/grpc.h

@@ -349,6 +349,15 @@ typedef struct grpc_op {
  } data;
 } grpc_op;

+#define GRPC_INHERIT_DEADLINE 1


INHERIT => PROPAGATE maybe?

I like that better... done.

jtattermusch · 2015-07-31T23:53:46Z

I took a brief look and it should be fairly easy to use in C#.

Conflicts: src/core/surface/channel.c

a-veitch · 2015-08-02T23:12:10Z

include/grpc/grpc.h

@@ -351,6 +351,19 @@ typedef struct grpc_op {
  } data;
 } grpc_op;

+/* Propagation bits: this can be bitwise or-ed to form propagation_mask for


What about auth context? Is that propagatable as well?

Yeah... I wanted to talk to @jboeuf before I went ahead and added it - and I didn't get a chance yet (fallout from late-on-a-Friday-afternoon-decision-making-in-my-cubicle).

a-veitch · 2015-08-03T15:23:13Z

include/grpc/grpc.h

+/** Propagate deadline */
+#define GRPC_PROPAGATE_DEADLINE ((gpr_uint32)1)
+/** Propagate census context */
+#define GRPC_PROPAGATE_CENSUS_CONTEXT ((gpr_uint32)2)


This flag is somewhat redundant with the census state, as reflected in the return value from census_enabled() - this is what I was intending to use as a signal for whether we should do context propagation or not. We can certainly have both signals, but it could be confusing.

census_enabled is global. What if I want to make one RPC without tracing propagation (but perhaps with deadline propagation?)

(This API is covering the grpc_call_set_census_context equivalent API)

You asked: What if I want to make one RPC without tracing propagation (but perhaps with deadline propagation?)

Good question. This would seem to be the best way of doing this (via updating the trace_mask when we start the operation). It raises another issue though, in that we use the census context for more than just tracing - what if I want to disable tracing, but keep stats collection enabled for this request? There are probably a couple of options here:

Use the GRPC_PROPAGATE_CENSUS_CONTEXT flag to {en/dis}able all features of census that require propagation.

Explicitly state that the GRPC_PROPAGATE_CENSUS_CONTEXT flag can only be used to disable tracing (leaving other census features that require propagation untouched).

Add GRPC_PROPAGATE_CENSUS_CONTEXT_X flags for the other census features (RPC stats + CPU accounting) that are done per-context.

3 would be my preference.

Could we find a half-way point perhaps?

GRPC_PROPAGATE_STATS
GRPC_PROPAGATE_TRACING

On Mon, Aug 3, 2015 at 9:14 AM Alistair Veitch notifications@github.com
wrote:

In include/grpc/grpc.h
#2750 (comment):

@@ -351,6 +351,23 @@ typedef struct grpc_op {
} data;
} grpc_op;

+/* Propagation bits: this can be bitwise or-ed to form propagation_mask for

* grpc_call /
+/* Propagate deadline /
+#define GRPC_PROPAGATE_DEADLINE ((gpr_uint32)1)
+/* Propagate census context */
+#define GRPC_PROPAGATE_CENSUS_CONTEXT ((gpr_uint32)2)

You asked: What if I want to make one RPC without tracing propagation (but
perhaps with deadline propagation?)

Good question. This would seem to be the best way of doing this (via
updating the trace_mask when we start the operation). It raises another
issue though, in that we use the census context for more than just tracing

what if I want to disable tracing, but keep stats collection enabled for
this request? There are probably a couple of options here:

Use the GRPC_PROPAGATE_CENSUS_CONTEXT flag to {en/dis}able all features
of census that require propagation.

Explicitly state that the GRPC_PROPAGATE_CENSUS_CONTEXT flag can only
be used to disable tracing (leaving other census features that require
propagation untouched).

Add GRPC_PROPAGATE_CENSUS_CONTEXT_X flags for the other census features
(RPC stats + CPU accounting) that are done per-context.

3 would be my preference.

—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc/pull/2750/files#r36101297.

SGTM.

Could we find a half-way point perhaps?

GRPC_PROPAGATE_STATS
GRPC_PROPAGATE_TRACING

I don't suppose you'd want to send me a PR against this one implementing
it? :)

On Mon, Aug 3, 2015 at 9:36 AM Alistair Veitch notifications@github.com
wrote:

In include/grpc/grpc.h
#2750 (comment):

@@ -351,6 +351,23 @@ typedef struct grpc_op {
} data;
} grpc_op;

+/* Propagation bits: this can be bitwise or-ed to form propagation_mask for

* grpc_call /
+/* Propagate deadline /
+#define GRPC_PROPAGATE_DEADLINE ((gpr_uint32)1)
+/* Propagate census context */
+#define GRPC_PROPAGATE_CENSUS_CONTEXT ((gpr_uint32)2)

SGTM.

Could we find a half-way point perhaps?

GRPC_PROPAGATE_STATS
GRPC_PROPAGATE_TRACING

—

Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc/pull/2750/files#r36103509.

I don't suppose you'd want to send me a PR against this one implementing it? :)

I can - but I think it will have to wait till we get #2668 figured out to do it "right". Which reminds me, I need to go and update that PR...

@jboeuf

Currently the oauth2 variant of these tests is disabled. Will work with @jboeuf to figure out how to turn them on.

ctiller · 2015-08-04T15:22:14Z

Added a test suite to this for the core library. Now most end2end tests have a variant that proxies calls and propagates context using this API.

ctiller · 2015-08-05T19:13:52Z

This is ready for review.

nathanielmanistaatgoogle · 2015-08-05T19:34:58Z

include/ and src/python/ changes LGTM.

a-veitch · 2015-08-05T20:26:44Z

include/grpc/grpc.h

+/** Propagate deadline */
+#define GRPC_PROPAGATE_DEADLINE ((gpr_uint32)1)
+/** Propagate census context */
+#define GRPC_PROPAGATE_STATS_CONTEXT ((gpr_uint32)2)


Since we're using the census name everywhere else, does it make sense to rename these similar to GRPC_PROPAGATE_CENSUS_STATS_CONTEXT? (I don't mind either way myself, just think of future readers of the code)

jcanizales · 2015-08-05T22:33:15Z

Mind adding NULL, GRPC_PROPAGATE_DEFAULTS, here: https://github.com/grpc/grpc/blob/master/src/objective-c/GRPCClient/private/GRPCWrappedCall.m#L249 ? I can send you a PR if you prefer, although it's just one line :)

ctiller · 2015-08-05T23:05:26Z

One of these days I'll commit that I need to include '*.m' in global search replaces to memory... done.

a-veitch · 2015-08-05T23:34:11Z

LG to me.

yang-g · 2015-08-06T05:24:52Z

include/grpc/grpc.h

+   propagation. Doing so gives flexibility in the future to define new 
+   propagation types that are default inherited or not. */
+#define GRPC_PROPAGATE_DEFAULTS                        \
+  ((gpr_uint32)((0xffff | GRPC_PROPAGATE_DEADLINE |    \


Why do we do the | GRPC_PROPAGATE_X again? Does it always end up to 0xffff?

It doesn't change the result, but I wanted to explicit:

the bits that default to on

and what they mean in terms of what gets propagated

ctiller · 2015-08-06T16:33:05Z

Green enough I think

Core-supported context propagation

Core-supported context inheritance sketch

3e7c6a7

googlebot added the cla: yes label Jul 31, 2015

jtattermusch reviewed Jul 31, 2015
View reviewed changes

ctiller added 3 commits July 31, 2015 17:01

Update wrappers, tests to new create_call()

99e6164

s/inherit/propagate/g

e1b0e6e

Add documentation outline

5847177

ctiller added this to the Beta milestone Aug 1, 2015

ctiller added area/core lang/all wrapped languages area/api/BREAKAGE labels Aug 1, 2015

ctiller added 2 commits August 1, 2015 16:11

Merge github.com:grpc/grpc into propagate

fa44f18

Conflicts: src/core/surface/channel.c

Merge branch 'propagate' of github.com:ctiller/grpc into propagate

6728bef

Conflicts: src/core/surface/channel.c

a-veitch reviewed Aug 2, 2015
View reviewed changes

Implement cancellation propagation, define auth propagation

c7df0df

a-veitch reviewed Aug 3, 2015
View reviewed changes

Add proxy tests to end2end suite

17effab

Currently the oauth2 variant of these tests is disabled. Will work with @jboeuf to figure out how to turn them on.

ctiller changed the title ~~Core-supported context inheritance sketch~~ Core-supported context propagation Aug 4, 2015

Merge github.com:grpc/grpc into propagate

18541a1

ctiller mentioned this pull request Aug 5, 2015

Reduce mac load on Jenkins #2800

Merged

ctiller added 4 commits August 5, 2015 10:43

Fix proxy, finalize API

402acf6

Cleanup unnecessary test

9974078

Merge github.com:grpc/grpc into propagate

a600d58

Merge github.com:grpc/grpc into propagate

22de7b6

ctiller assigned yang-g Aug 5, 2015

ctiller added 2 commits August 5, 2015 12:47

Merge github.com:grpc/grpc into propagate

5ade369

Fix integration breakage

64f32dc

a-veitch reviewed Aug 5, 2015
View reviewed changes

ctiller added 2 commits August 5, 2015 15:24

Rename constants

fdbd728

Add comment

a6f8f92

ctiller mentioned this pull request Aug 5, 2015

C++ propagation API #2813

Merged

Update Objective-C

354e212

yang-g reviewed Aug 6, 2015
View reviewed changes

Explicitly spell out cancellation in default propagation mask

44912d7

yang-g added a commit that referenced this pull request Aug 6, 2015

Merge pull request #2750 from ctiller/propagate

e5b0459

Core-supported context propagation

yang-g merged commit e5b0459 into grpc:master Aug 6, 2015

murgatroid99 mentioned this pull request Aug 11, 2015

Ensure census context can be passed from incoming requests to outgoing requests in Ruby #2517

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core-supported context propagation #2750

Core-supported context propagation #2750

ctiller commented Jul 31, 2015

jtattermusch Jul 31, 2015

ctiller Aug 1, 2015

jtattermusch commented Jul 31, 2015

a-veitch Aug 2, 2015

ctiller Aug 3, 2015

a-veitch Aug 3, 2015

ctiller Aug 3, 2015

ctiller Aug 3, 2015

a-veitch Aug 3, 2015

ctiller Aug 3, 2015

a-veitch Aug 3, 2015

ctiller Aug 3, 2015

a-veitch Aug 4, 2015

ctiller commented Aug 4, 2015

ctiller commented Aug 5, 2015

nathanielmanistaatgoogle commented Aug 5, 2015

a-veitch Aug 5, 2015

ctiller Aug 5, 2015

jcanizales commented Aug 5, 2015

ctiller commented Aug 5, 2015

a-veitch commented Aug 5, 2015

yang-g Aug 6, 2015

ctiller Aug 6, 2015

ctiller commented Aug 6, 2015

Core-supported context propagation #2750

Core-supported context propagation #2750

Conversation

ctiller commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch commented Jul 31, 2015

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

Choose a reason for hiding this comment

ctiller commented Aug 4, 2015

ctiller commented Aug 5, 2015

nathanielmanistaatgoogle commented Aug 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcanizales commented Aug 5, 2015

ctiller commented Aug 5, 2015

a-veitch commented Aug 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctiller commented Aug 6, 2015