Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused method in ContextStorage #2699

Closed
bogdandrutu opened this issue Feb 4, 2021 · 9 comments
Closed

Unused method in ContextStorage #2699

bogdandrutu opened this issue Feb 4, 2021 · 9 comments
Labels
Bug Something isn't working

Comments

@bogdandrutu
Copy link
Member

static void addWrapper(Function<? super ContextStorage, ? extends ContextStorage> wrapper) {

I think this should be removed until concrete use-case available

@bogdandrutu bogdandrutu added the Bug Something isn't working label Feb 4, 2021
@anuraaga
Copy link
Contributor

anuraaga commented Feb 4, 2021

This was added for two users I think, one JFR, and is demonstrated in the JFR test. It follows my experience in Armeria too where the SPI just too tedious for some trivial use cases.

@bogdandrutu
Copy link
Member Author

Do you have a real example of a usage of the Armeria API?

@anuraaga
Copy link
Contributor

anuraaga commented Feb 4, 2021

Here's the original Armeria issue and PR line/armeria#2634 line/armeria#2723

Unfortunately can't find any OSS usages. /cc @minwoox @ikhoon @trustin

@trustin
Copy link

trustin commented Feb 4, 2021

As far as I know, LINE corporation uses that feature internally. @minwoox or @ikhoon could give some concrete example.

@bogdandrutu
Copy link
Member Author

@anuraaga I see, but it seems to have very limited usage. Can we postpone adding it until a real user asks us to do it.

@anuraaga
Copy link
Contributor

anuraaga commented Feb 5, 2021

@bogdandrutu @sfriberg @HaloFour Users asked for it, that's why we added it

@HaloFour
Copy link
Contributor

HaloFour commented Feb 5, 2021

I am actively using this API now so that I can determine when the current span changes so that I can update MDC and have the span information included in the logs.

I am using this in a Java Spring WebFlux application that is using the OpenTelemetry SDK as well as in a separate Scala tracing library that I've recently adopted to implementing the OpenTelemetry API interfaces and to use Context.

Here's some of where that Scala library relies on this API:

https://github.com/Comcast/money/blob/master/money-core/src/main/scala/com/comcast/money/core/Money.scala#L79
https://github.com/Comcast/money/blob/master/money-core/src/main/scala/com/comcast/money/core/context/MdcContextStorageFilter.scala

@minwoox
Copy link

minwoox commented Feb 5, 2021

Sorry for the late reply. 😅
At LINE corporation, there's a service that uses Tomcat and Armeria together. (The service was originally built using Tomcat and add Armeria server with a different port.)

They used to have their own logic for request scoping using thread-local. So they can log MDC parameters and retrieve some instances they store in the thread-local.
It used to work well until they decided to use asynchronous client and server with the Tomcat together.
They couldn't do the request scoping for the requests that are served from the asynchronous server.

So we decide to add the sort of hook so that they could solve the problem. 😄

@bogdandrutu
Copy link
Member Author

Ok.. too late then 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants