Restrict access to context key, add helper methods to interact with Context. #1864

songy23 · 2019-04-25T19:48:25Z

Similar to open-telemetry/opentelemetry-java#211.

songy23 · 2019-04-25T19:49:16Z

api/src/main/java/io/opencensus/trace/unsafe/ContextUtils.java

+  @Deprecated
  public static final Context.Key</*@Nullable*/ Span> CONTEXT_SPAN_KEY =
-      Context.key("opencensus-trace-span-key");
+      Context.<Span>key("opencensus-trace-span-key");


Default value for context span key is still null for backwards-compatibility.

Who needs it? gRPC does not need to be null. We don't change the key but the result of unsafe getValue can apply default logic

gRPC expected this behavior in their tests, e.g https://github.com/grpc/grpc-java/blob/master/core/src/test/java/io/grpc/internal/CensusModulesTest.java#L306. I'm not sure if this is used anywhere else. /cc @zhangkun83

But we don't change the behavior for the key, only for the new API which we can implement whatever we want.

dinooliva · 2019-04-25T20:06:15Z

api/src/main/java/io/opencensus/tags/unsafe/ContextUtils.java

+   * @return a new context with the given value set.
+   * @since 0.21
+   */
+  public static Context withValue(Context context, TagContext tagContext) {


Shouldn't you check whether tagContext is null? Or annotate as nullable?

No, because the key is initialized with a default value.

Reverted, though I'm wondering when we would want to set TagContext or Span to null in a context.

Agreed that the default value does mean the tagContext can be null but still not clear to me that there is a good use-case for this.

If there is, shouldn't it be annotated Nullable?

I added the Nullable annotation to the context value. The new API still returns non-null value though.

dinooliva · 2019-04-25T20:09:27Z

api/src/main/java/io/opencensus/tags/unsafe/ContextUtils.java

+   * @return the value from the specified {@code Context}.
+   * @since 0.21
+   */
+  public static TagContext getValue(Context context) {


Maybe these names should be less generic since they are doing specific work - e.g. withTagContext and getTagContext

I would keep them as they are to not make confusion with the main APIs.

bogdandrutu · 2019-04-25T20:19:38Z

api/src/main/java/io/opencensus/tags/unsafe/ContextUtils.java

+   * @since 0.21
+   */
+  public static Context withValue(Context context, TagContext tagContext) {
+    return Utils.checkNotNull(context, "context").withValue(TAG_CONTEXT_KEY, Utils.checkNotNull(tagContext, "tagContext"));


tagContext may be null. no need to check.

bogdandrutu · 2019-04-25T20:20:32Z

api/src/main/java/io/opencensus/trace/unsafe/ContextUtils.java

+  // TODO(songy23): make this private once gRPC migrates to use the alternative APIs.
+  @Deprecated
+  public static final Context.Key<Span> CONTEXT_SPAN_KEY =
+      Context.<Span>keyWithDefault("opencensus-trace-span-key", BlankSpan.INSTANCE);


we cannot change the default for the key.

Makes sense, fixed.

bogdandrutu · 2019-04-25T20:20:42Z

api/src/main/java/io/opencensus/trace/unsafe/ContextUtils.java

-  public static final Context.Key</*@Nullable*/ Span> CONTEXT_SPAN_KEY =
-      Context.key("opencensus-trace-span-key");
+  public static Span getValue(Context context) {
+    return CONTEXT_SPAN_KEY.get(Utils.checkNotNull(context, "context"));


here we implement the default logic.

bogdandrutu · 2019-04-25T20:21:08Z

api/src/main/java/io/opencensus/trace/unsafe/ContextUtils.java

+   * @since 0.21
+   */
+  public static Context withValue(Context context, Span span) {
+    return Utils.checkNotNull(context, "context").withValue(CONTEXT_SPAN_KEY, Utils.checkNotNull(span, "span"));


no need to check for not null for the span, because we can support null Span correct?

songy23

Reverted 6acf9d9 in favor of .799a838

songy23 · 2019-04-25T20:39:29Z

api/src/main/java/io/opencensus/tags/unsafe/ContextUtils.java

+   * @return a new context with the given value set.
+   * @since 0.21
+   */
+  public static Context withValue(Context context, TagContext tagContext) {


Reverted, though I'm wondering when we would want to set TagContext or Span to null in a context.

songy23 · 2019-04-25T20:39:35Z

api/src/main/java/io/opencensus/tags/unsafe/ContextUtils.java

+   * @since 0.21
+   */
+  public static Context withValue(Context context, TagContext tagContext) {
+    return Utils.checkNotNull(context, "context").withValue(TAG_CONTEXT_KEY, Utils.checkNotNull(tagContext, "tagContext"));


songy23 · 2019-04-25T20:39:49Z

api/src/main/java/io/opencensus/trace/unsafe/ContextUtils.java

+  // TODO(songy23): make this private once gRPC migrates to use the alternative APIs.
+  @Deprecated
+  public static final Context.Key<Span> CONTEXT_SPAN_KEY =
+      Context.<Span>keyWithDefault("opencensus-trace-span-key", BlankSpan.INSTANCE);


Makes sense, fixed.

songy23 · 2019-04-25T20:40:14Z

api/src/main/java/io/opencensus/trace/unsafe/ContextUtils.java

+   * @since 0.21
+   */
+  public static Context withValue(Context context, Span span) {
+    return Utils.checkNotNull(context, "context").withValue(CONTEXT_SPAN_KEY, Utils.checkNotNull(span, "span"));


songy23 · 2019-04-25T20:40:31Z

api/src/main/java/io/opencensus/trace/unsafe/ContextUtils.java

-  public static final Context.Key</*@Nullable*/ Span> CONTEXT_SPAN_KEY =
-      Context.key("opencensus-trace-span-key");
+  public static Span getValue(Context context) {
+    return CONTEXT_SPAN_KEY.get(Utils.checkNotNull(context, "context"));


Also updated CensusModule to use the new helper methods ContextUtils.withValue() instead of directly manipulating the context keys. See census-instrumentation/opencensus-java#1864.

Restrict access to the key, add helper methods to interact with Context.

18a9977

songy23 added gRPC API context labels Apr 25, 2019

songy23 added this to the Release 0.21.0 milestone Apr 25, 2019

songy23 requested a review from bogdandrutu April 25, 2019 19:48

songy23 requested review from a team, dinooliva and rghetia as code owners April 25, 2019 19:48

googlebot added the cla: yes label Apr 25, 2019

songy23 commented Apr 25, 2019

View reviewed changes

Fix a test.

a0912f6

bogdandrutu approved these changes Apr 25, 2019

View reviewed changes

dinooliva reviewed Apr 25, 2019

View reviewed changes

bogdandrutu reviewed Apr 25, 2019

View reviewed changes

Update ContextUtils.getValue to return non-null. Add to changelog.

799a838

songy23 force-pushed the context-key branch from 6acf9d9 to 799a838 Compare April 25, 2019 20:44

songy23 commented Apr 25, 2019

View reviewed changes

Fix tests.

eb535cf

bogdandrutu approved these changes Apr 25, 2019

View reviewed changes

songy23 added 2 commits April 25, 2019 15:21

Add Nullable annotation for TagContext.

e04e306

Two more Nullable annotatiions.

459bb9d

dinooliva approved these changes Apr 26, 2019

View reviewed changes

songy23 merged commit 916adcb into census-instrumentation:master Apr 26, 2019

songy23 deleted the context-key branch April 26, 2019 00:20

songy23 mentioned this pull request May 1, 2019

all: Upgrade OpenCensus versions. grpc/grpc-java#5657

Merged

Restrict access to context key, add helper methods to interact with Context. #1864

Restrict access to context key, add helper methods to interact with Context. #1864

Uh oh!

Conversation

songy23 commented Apr 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bogdandrutu Apr 25, 2019 •

edited

Loading