Provide a way to customize context propagation using its own storage #2610

minwoox · 2020-03-19T02:39:58Z

Motivation:
Related #2375 (comment)
It will be good if we provide a way to customize the context storage so that a user can do the scope management by him/herself.

Modifications:

Add RequestContextStorage.
Add RequestContextStorageProvier so that a user customizes the ContextStorage using Java SPI

Result:

You can, now, create your own storage to store RequestContext.
Close Consider adding hooks back to RequestContext #2514

Motivation: Modifications: - Add `ContextStorage` and its default implementation `ContextStorageThreadLocal`. - Add `ContextStorageProvier` so that a user customizes the `ContextStorage` using Java SPI Result:

minwoox · 2020-03-19T02:40:17Z

I need to add Javadoc, but I wanted to get some feedback especially for naming before I finish. PTAL

anuraaga

Basically looks good, thanks!

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java

core/src/main/java/com/linecorp/armeria/common/ContextStorage.java

core/src/main/java/com/linecorp/armeria/common/ContextStorageThreadLocal.java

core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java

trustin

@anuraaga already commented everything important, so not much to add. 😅

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

minwoox · 2020-03-19T08:03:35Z

@anuraaga and @trustin, I updated. PTAL. 🙇
Will update the commit message later.

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

anuraaga · 2020-03-19T09:13:28Z

core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java

+                ServiceLoader.load(ContextStorageProvider.class));
+        final String contextStorageFqcn = Flags.contextStorage();
+        if (!providers.isEmpty()) {
+            if (providers.size() > 1) {


Ah I was thinking instead of loading directly from the flag, we'd use the flag to disambiguate among the different providers in this case. I guess we can do both though

Ah, I guess I totally misunderstood. 🤣
So you meant when multiple providers exist (e.g com.x.AProvider, io.x.BProvider...), if the flag is io.x.BProvider, then we should choose the BProvider class. Right?

If so, I want to get rid of the current way and follow it because, I think, there would be a case who wants to use two different JARs that have their own RequestContextStorage.
So I think we should choose one of them instead of throwing an exception. 😉

core/src/main/java/com/linecorp/armeria/common/ContextStorage.java

core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java

core/src/main/java/com/linecorp/armeria/internal/common/ThreadLocalContextStorage.java

core/src/main/java/com/linecorp/armeria/common/Flags.java

codecov · 2020-03-19T13:15:29Z

Codecov Report

Merging #2610 into master will decrease coverage by 0.06%.
The diff coverage is 59.03%.

@@             Coverage Diff              @@
##             master    #2610      +/-   ##
============================================
- Coverage     73.35%   73.29%   -0.07%     
- Complexity    11047    11060      +13     
============================================
  Files           971      972       +1     
  Lines         42518    42588      +70     
  Branches       5294     5309      +15     
============================================
+ Hits          31191    31216      +25     
- Misses         8617     8658      +41     
- Partials       2710     2714       +4

Impacted Files	Coverage Δ	Complexity Δ
...rp/armeria/internal/common/RequestContextUtil.java	`46.66% <30.43%> (-53.34%)`	`12.00 <7.00> (+4.00)`	⬇️
...meria/common/ThreadLocalRequestContextStorage.java	`87.50% <87.50%> (ø)`	`4.00 <4.00> (?)`
.../linecorp/armeria/client/ClientFactoryBuilder.java	`65.85% <100.00%> (+0.42%)`	`42.00 <1.00> (+1.00)`
.../linecorp/armeria/client/ClientRequestContext.java	`70.00% <100.00%> (ø)`	`22.00 <9.00> (+1.00)`
...c/main/java/com/linecorp/armeria/common/Flags.java	`61.77% <100.00%> (+0.44%)`	`61.00 <1.00> (+1.00)`
...va/com/linecorp/armeria/common/RequestContext.java	`62.96% <100.00%> (-1.33%)`	`19.00 <3.00> (-1.00)`
...linecorp/armeria/common/RequestContextStorage.java	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
...ava/com/linecorp/armeria/server/ServerBuilder.java	`79.61% <100.00%> (+0.11%)`	`105.00 <0.00> (ø)`
...linecorp/armeria/server/ServiceRequestContext.java	`73.07% <100.00%> (ø)`	`22.00 <7.00> (+1.00)`
.../java/com/linecorp/armeria/client/HttpSession.java	`41.17% <0.00%> (-5.50%)`	`5.00% <0.00%> (ø%)`
... and 20 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 8e0cdb1...ccf427a. Read the comment docs.

core/src/main/java/com/linecorp/armeria/common/RequestContextStorage.java

core/src/main/java/com/linecorp/armeria/internal/common/ThreadLocalRequestContextStorage.java

core/src/main/java/com/linecorp/armeria/common/Flags.java

core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java

ikhoon

Thanks a lot! @minwoox 👍

anuraaga

Thanks, looks great

core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java

trustin · 2020-03-20T13:49:49Z

core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java

+            } catch (Throwable t) {
+                throw new IllegalStateException("Failed to create context storage. provider: " + provider, t);
+            }
+            logger.info("{} is used to create the request context storage: {}",


I actually meant not logging when:

using the default provider (because it may be noisy.)

specified via the flag (because it's logged there already.)

i.e. we can log in the else {} block above.

I thought it might be useful to show the information of the created request context storage after it's fully created. (i.e. no exception when calling newStorage()).

But I guess showing the created storage context is too verbose. Fixed. Thank you!

minwoox · 2020-03-25T01:08:27Z

Thanks for reviewing!

…e#2610) Motivation: Related line#2375 (comment) It will be good if we provide a way to customize the context storage so that a user can do the scope management by him/herself. Modifications: - Add `RequestContextStorage`. - Add `RequestContextStorageProvier` so that a user customizes the `ContextStorage` using Java SPI Result: - You can, now, create your own storage to store `RequestContext`. - Close line#2514

Provide a way to customize context propagation using its own storage

5488ed0

Motivation: Modifications: - Add `ContextStorage` and its default implementation `ContextStorageThreadLocal`. - Add `ContextStorageProvier` so that a user customizes the `ContextStorage` using Java SPI Result:

minwoox added the new feature label Mar 19, 2020

minwoox added this to the 0.99.0 milestone Mar 19, 2020

minwoox requested review from trustin, anuraaga and ikhoon March 19, 2020 02:39

anuraaga reviewed Mar 19, 2020

View reviewed changes

trustin reviewed Mar 19, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java Outdated Show resolved Hide resolved

Address comments by @anuraaga and @trustin

b9c59b6

minwoox force-pushed the add_context_storage branch from 1897d23 to b9c59b6 Compare March 19, 2020 07:59

Remove useless method

3472811

minwoox marked this pull request as ready for review March 19, 2020 08:02

anuraaga reviewed Mar 19, 2020

View reviewed changes

trustin reviewed Mar 19, 2020

View reviewed changes

minwoox added 4 commits March 19, 2020 21:52

Address comment by @anuraaga and @trustin

cf3dd19

Fix

10dab88

Fix 2

530e2d5

Address the comment by @trustin

9fd12e5

anuraaga approved these changes Mar 19, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/RequestContextStorage.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/internal/common/ThreadLocalRequestContextStorage.java Outdated Show resolved Hide resolved

trustin reviewed Mar 19, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/Flags.java Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java Show resolved Hide resolved

Address comments by @anuraaga and @trustin

1f57707

ikhoon reviewed Mar 20, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java Show resolved Hide resolved

ikhoon approved these changes Mar 20, 2020

View reviewed changes

anuraaga approved these changes Mar 20, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java Outdated Show resolved Hide resolved

minwoox added 2 commits March 20, 2020 16:39

enum singleton

cdec669

Fix Javadoc

6e415d9

trustin reviewed Mar 20, 2020

View reviewed changes

minwoox and others added 2 commits March 20, 2020 23:25

Fix logging

9b9b64a

Update RequestContextUtil.java

63ffc15

trustin approved these changes Mar 23, 2020

View reviewed changes

Update RequestContextUtil.java

ccf427a

trustin merged commit b297e73 into line:master Mar 25, 2020

minwoox deleted the add_context_storage branch March 25, 2020 01:08

codefromthecrypt mentioned this pull request Apr 5, 2020

remove grpc-context dependency from the api package open-telemetry/opentelemetry-java#575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to customize context propagation using its own storage #2610

Provide a way to customize context propagation using its own storage #2610

minwoox commented Mar 19, 2020 •

edited

Loading

minwoox commented Mar 19, 2020

anuraaga left a comment

trustin left a comment

minwoox commented Mar 19, 2020

anuraaga Mar 19, 2020

minwoox Mar 19, 2020

codecov bot commented Mar 19, 2020 •

edited

Loading

ikhoon left a comment

anuraaga left a comment

trustin Mar 20, 2020

minwoox Mar 20, 2020

minwoox Mar 20, 2020

minwoox commented Mar 25, 2020

Provide a way to customize context propagation using its own storage #2610

Provide a way to customize context propagation using its own storage #2610

Conversation

minwoox commented Mar 19, 2020 • edited Loading

minwoox commented Mar 19, 2020

anuraaga left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

minwoox commented Mar 19, 2020

anuraaga Mar 19, 2020

Choose a reason for hiding this comment

minwoox Mar 19, 2020

Choose a reason for hiding this comment

codecov bot commented Mar 19, 2020 • edited Loading

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

trustin Mar 20, 2020

Choose a reason for hiding this comment

minwoox Mar 20, 2020

Choose a reason for hiding this comment

minwoox Mar 20, 2020

Choose a reason for hiding this comment

minwoox commented Mar 25, 2020

minwoox commented Mar 19, 2020 •

edited

Loading

codecov bot commented Mar 19, 2020 •

edited

Loading