Opentelemetry-java: remove grpc-context dependency from the api package

[adriancole]

I understand why opentelemetry has a bias towards google's gRPC library. This codebase primarily borrows from work that started internally, became google's instrumentation-java, later renamed to opencensus, and now mostly copy/pasted from that. The original primary customer was not just gRPC protocol, but specifically google's library. For example, gRPC is the most primary user of the prior work, and it is helpful to have a user to guide.

Time has passed, though, and what started as an end-to-end instrumentation library has split into a Api + SDK approach, as facilitated by the weaving in of OpenTracing. Yet, code opinons towards google's gRPC implementation still exist in the form of a strict dependency in the api package, and helper code.

While it is understandable that alliances exists, and having good integration with Google's gRPC implementation is important, even gRPC itself has options. For example, Armeria has gRPC protocol support without the need of Google's library. We have already encountered problems where other google projects drag in gRPC because of Census. This should stop now. OpenTelemetry as a body of people, sure that can advocate for Google. However, the instrumentation api must not pick winners and losers in RPC frameworks.

Please remove all traces of grpc-context from the api library and move the support functions to a context integration package.

This is a follow-up from here: https://github.com/open-telemetry/opentelemetry-java/pull/571#issuecomment-536193532

[adriancole]

here are some related issues from grpc-context:

• 2016 pulling grpc-context into a different library, but still in grpc repo
• 2017 modularity problems master issue including Java 9 modules and also OSGi

[adriancole]

It may also not be obvious why removing it from the api package is important besides bloat. grpc-context is not even an optional dependency. strict dependencies not only cause bloat and pin versions, but they also obscure what apis are actually powering the software. This means it hides abstraction leaks. For example, you can't tell if an instrumentation site requires grpc-context or not by looking at its dependencies: you have to scan all of its code.

TL;DR; Currently, anything marked as opentelemetry-api might as well be assumed to only work when grpc-context is present.

$ ./gradlew :opentelemetry-api:dependencies
--snip--

compileClasspath - Compile classpath for source set 'main'.
+--- com.google.auto.value:auto-value-annotations:1.6.2
+--- com.google.errorprone:error_prone_annotations:2.3.2
+--- com.google.code.findbugs:jsr305:3.0.2
\--- io.grpc:grpc-context:1.20.0

--snip--

runtimeClasspath - Runtime classpath of source set 'main'.
+--- com.google.auto.value:auto-value-annotations:1.6.2
+--- com.google.errorprone:error_prone_annotations:2.3.2
+--- com.google.code.findbugs:jsr305:3.0.2
\--- io.grpc:grpc-context:1.20.0

[adriancole]

See also #576 as the SDK package is far worse

[trask]

@carlosalberto @bogdandrutu @jkwatson I'm worried that we went in the opposite direction of removing the grpc-context dependency in 0.3.0, and it is now part of the public API surface in 0.3.0, with OpenTelemetry API users interacting directly with gRPC Context.

Summarizing my thoughts below about the grpc-context dependency, and I will join the Java SIG meeting tomorrow morning so we can discuss in person.

1. The dependency on grpc-context makes it less clear what the OpenTelemetry API surface really is.

   For example, the OpenTelemetry API's current span can be manipulated directly via gRPC Context. Is this part of OpenTelemetry API surface?

   For another example, the way to create an OpenTelemetry scope listener currently is by creating a class with a very special name io.grpc.override.ContextStorageOverride. This seems like a very odd public API surface that we are exposing. (and what happens if there is already one of these classes on the classpath?)

2. The dependency on grpc-context causes a particular problem for the auto-instrumentation project.

   We instrument the OpenTelemetry API itself, so that telemetry sent from an application/library to OpenTelemetry API will seamlessly interoperate with telemetry captured by auto-instrumentation. If gRPC Context is part of the OpenTelemetry API surface, we will have to interoperate with all features (and probably multiple versions) of gRPC Context, in order to keep the "application-space" gRPC Context in sync with our embedded SDK's gRPC Context.

3. I think we will have better success getting popular libraries to emit telemetry using opentelemetry-api if it does not pull in grpc-context.

   Many libraries are super conservative with introducing new dependencies. I already heard resistance from the Azure SDK team this week when they went to upgrade to 0.3.0 and saw the hard dependency on gRPC Context (see https://github.com/Azure/azure-sdk-for-java/issues/9695). I don't think this will be an uncommon concern.

[tedsuo]

I would prefer this dependency be removed. I believe the gPRC team was open to moving to a shared context dependency. I feel this would be the right move - both projects sharing a stand-alone context package. This was one of the intentions behind the move to separate context propagation from tracing.

The other approach would be to ask the grpc team to take on an opentelemetry dependency.

@bogdandrutu I know you were interested in this, and already had it on your radar. Is now the time?

[Oberon00]

See https://github.com/open-telemetry/opentelemetry-java/pull/720#discussion_r371776798

[trask]

We should also consider Project Reactor's Context, which is used by Spring WebFlux.

Project Reactor and Spring WebFlux are also very popular projects in the Java world.

So I don't think that gRPC Context is the "clear winner" in the context propagation space, which seems to be the requirement for us to pick an external library for context propagation.

And I love the idea of us working with gRPC, Project Reactor and others to help create a "clear winner" in the Java context propagation space that we can all take a dependency on.

But in the meantime, let's replace gRPC Context with our own Context implementation, and let's see if it's possible to avoid exposing our Context object directly in our public API surface so that we can swap out our Context implementation with a "clear winner" some time in the future.

[adriancole]

copying in @carlosalberto who seems to be the one who decided to make a hard dependency based on bogdan's advice.

Note: I think the maintainers of this repo should be revisited, as it should really be led by folks who are conservative about things like this by default.

[adriancole]

by revisit, I mean that whoever is a maintainer acknowledges that they won't add any hard dependency on a 3rd party library for paths required for usage. And especially this point..

when you do decide to do the opposite of a pending issue, you must comment on that issue. Avoiding conflict by not doing that isn't good for the project as it leads to surprise.

[jkwatson]

Hi @adriancole . Newly minted maintainer here, trying to make sure this gets addressed. I wholeheartedly agree that we need to minimize dependencies in the API layer, and that removing grpc-context from the API surface is absolutely our goal. Also, getting rid of grpc-context as a hard API dependency is also needed.

The other desire, I believe, is to have a battle-hardened context implementation in opentelemetry, so that we can launch with it.

Do you have a constructive suggestion on what we should be using for our context implementation?

I can see several options:
1) bring in an existing production-ready, appropriately licensed context implementation into the codebase as source-code, repackaged under an opentelemetry namespace.
2) build a brand-new context implementation into opentelemetry, which isn't production-hardened.
3) design an context abstraction, with no implementation, for the API, continuing to use a wrapped grpc-context as the implementation in the SDK.

I'm sure there are other options. I'd love to hear what your approach would be.

[adriancole]

@jkwatson thanks for asking! honestly, this is a good turn in itself that you are asking outsiders advice. bravo! Here's some experience from the zipkin brave project.

separate api for sure. you will need to make some decisions, particularly if it is immutable only. only add the apis you need, nothing more. here are some hints

• consider whether or not you want a strongly typed key. this seems nice but also limiting
  
  
  
  • strongly typed key at the surface of your api does not imply a strongly typed one underneath.
  
  
• probably stick to string values, and push any conversion into the users hand via decoration
  
  
  
  • very few propagated keys are numbers, and there's cost mostly on update which is likely infrequent.
  
  
• make a read-only api available. most call sites only need to see the current value (ex parent)
• consider impact of reactive, as immutable contexts are gnarly especially you can't set a value late.
  
  
  
  • @bsideup covers some of this https://bsideup.github.io/posts/daily_reactive/thread_locals/
  
  
• consider sometimes the context is embedded in a message. It is hard to add this api later.
  
  
  
  • @sjoerdtalsma has a lot of experience. more people should know him, and you should also ask him! https://github.com/talsma-ict/context-propagation
  
  

You will need an implementation plug-in that forwards minimally to a stacked thread local, but could also plug into other context storage apis. For example, gRPC has one (so does finagle). Armeria recently designed one also. Note.. some context storage is request scoped!

• @minwoox probably knows where some bodies are buried https://github.com/line/armeria/pull/2610

[adriancole]

@jkwatson since I have your ear, I'll give you unsolicited feedback on how to make friends with other projects. Some preceding work ripped off designs or even large amounts of code copy/pasted with no thanks or attribution. This really made people mad. When you do make a design or implementation, please in the PR thank them, and also cite somewhere in at least comments if not javadoc or a RATIONALE the inspiration. You can turn this culture around.

[jkwatson]

So, it sounds like your general approach would be to build something new, rather than work with something already battle-hardened?

[adriancole]

@jkwatson I assume you mean grpc context? It isn't a separate library, and also the feature scope is broader in some ways and more narrow than other context libraries. I think the api surface should be not tied to grpc context, but if the community decide to make that the default implementation they could (as long as the library is extracted, has no deps and doesn't change the JDK requirements). Basically there's still a decision about the api surface area even if you use gRPC by default, there are a lot of api decisions nested in grps, like timeout listeners etc that may not be appropriate here, and you also have to consider most RPC libraries control their own context and are not using grpc underneath.

hope this helps.

[adriancole]

the best way out is to be diverse in testing.. ex don't only test interop with grpc. test interop with several libraries (ex reactor-netty, armeria, non-rpc). This will show what I'm telling about library bias.

[adriancole]

here's a really old discussion that might be interesting https://github.com/DistributedTracing/continuation-local-storage-jvm/pull/1

also, if you need more wrenches for the clockwork, ask someone about akka :)

[adriancole]

Last unsolicited.. I might be wrong, but I think grpc has a bias towards static initial context lookup.

please do not expect everything to be static lookups. this was a problem in previous designs to make everything static registries.. "static only" is not an appropriate solution for DI or modular (OSGi environments) even if it can make sense for agents.

I can't find it now, but @jkschneider had a neat google doc or something on how micrometer allows choice of static or injected registries.

[jkwatson]

@jkwatson I assume you mean grpc context?

No, I was just trying figure out between the 2 BIG options: build something new, or use something that already exists, with proper attribution, etc.

[carlosalberto]

Hey @adriancole

No need to freak out here, as we are still before 1.0 and this issue has been closed ;) But thanks for the additional information. There have been interesting reasons on the advantages and disadvantages of grpc.Context, and we kept the dependency for now, while we figure out the way to go (either keep it or abstract in a specific way).

My initial prototype considered abstracting this part, and my slight personal preference would be to make grpc.Context and independent module/artifact that, so it would become the de facto context solution for Java (something similar to what contextvars is for Python).

In any case, as John suggested we will evaluate our options now. But definitely this will happen before 1.0 ;)

[jkwatson]

Document being used to capture a more dynamic discussion: https://docs.google.com/document/d/1dKXHYH1CQm4IQuRl5NZaoh2vETjRlBfhcsciz_Xs5hQ/edit?usp=sharing

[adriancole]

pre 1.0 libraries when marketed to masses end up in people's code, even
wise people's code sometimes. be careful is all. this is a serious problem.

[carlosalberto]

pre 1.0 libraries when marketed to masses end up in people's code

Disagree on this point, at least for OTel - observe we just released beta a few days ago, and as such, the term beta should signal people they can still expect changes.

Lets definitely focus on the technical discussion. So I suggest commenting on the document, so a decision can be done with all the advantages/disadvantages of each approach taken into account. I have no strong attachment to any alternative, so I'm open to what is agreed ;)

[Oberon00]

pre 1.0 libraries when marketed to masses end up in people's code

Can confirm. Planning with the initial release dates + some not generous enough buffer can lead to that. EDIT: To be clear, this was not entirely unexpected so it's not that bad; we are not re-exposing any interfaces and are prepared for breaking changes / sticking with old versions for some time.

[adriancole]

I noticed my notes didn't move into the doc so I added them

Knowing opentracing was pushed with marketing and its final version was 0.33.. and that census also was a dep of grpc and also google's http library also <1.0, I think folks dismissing the concern about 1.0 aren't maybe taking this seriously enough.

[adriancole]

the google doc seems to have a lot of active doubt that there isn't enough manpower etc to do context. Speaking from experience on a project which has never had more than one fulltime engineer, and most integrations are done by volunteers, I would pretty much say don't limit yourself.

Here's an example of integrating with an existing context using a bridge. there are edge cases around this, pros and cons, what to do if there's no request (ex fallback to a thread local?), but if a volunteer project can do this, so can open telemetry. give it a shot!

https://github.com/line/armeria/blob/master/brave/src/main/java/com/linecorp/armeria/common/brave/RequestContextCurrentTraceContext.java

[jkwatson]

@bogdandrutu @carlosalberto I'd love to have a proposal by the end of this week's SIG meeting on Friday, or at least a plan of action to test various options. Is there additional information/feedback that you'd like before you feel like we're in a good position to make that happen?

[andrewhsu]

From the SIG mtg today, sounds like more analysis after going through the gdoc:

• @bogdandrutu to put together an analysis of what changes to grpc would make it ideal to be used by opentelemetry-java
• @carlosalberto analysis of what is the min requirements for handling context if a custom api is written for opentelemetry-java

But from the conversations sounds like this issue is super desirable to be solved before GA.

[adriancole]

I'd like to see an explicit answer on this rather than rumors. If the decision is to depend on grpc-context, even if it is renamed, it will limit integration points in favor of grpc.

That the tradeoffs were listened to and understood should be documented in a RATIONALE that lives in this project if that was the outcome @bogdandrutu (mentioning you specifically as you are the most vocal in having this dependency)

[adriancole]

This decision is one of the most important decisions there is in this project as it limits or opens applicability in few ways except possibly a JRE version choice could. Propagation is the heart of tracing (and also metrics to the degree context tags are used)

A section in the RATIONALE saying "Here's why we use grpc-context" can reduce frustration, as no one would bother lobbying to decouple it or tentatively writing code wondering what will happen next. Moreover, folks who are told this is the "one api" can get a more honest assessment of where the "one" part starts and stops. This is open source after all, let's be open about this.

Even if the answer is grpc-context or some renaming of it, I presume agents would be less limited by this choice, for example, than tools that cannot affect the class loader (ex those statics). This isn't to say it is 100% wrong, just this is such a huge decision it must be transparent both with the pros and the cons, and not in google doc form.

[trask]

Hey @adriancole! Sorry for the lack of updates here. @bogdandrutu and I have been discussing, and spending time trying to figure this out. I could see this being worth a weekly 30 min meeting for a few weeks while we are working through ideas/options, would you be able to join if we schedule something that is timezone friendly for you?

[adriancole]

please just update this issue as you have updates, or point to something that is updated that I can watch? I'm not able to commit to synchronous meetings, but I can follow offline updates if it is known where they are.

[trask]

Should have something that's worth reviewing in the next day or two. Will post back here.

[trask]

Please review prototype over at https://github.com/propagationio/context/pull/1

[adriancole]

@trask you might consider reverting the src tree of the initial commit and then re-raising it as a PR. There are many decisions that predate the first pull request

[adriancole]

also, in that PR please have someone from grpc review it as it seems to be the idea that grpc would adopt this. anything that implies a new base class would imply its usefulness will be limited to adoptees, which was the problem in the initial discussion mentioned here https://github.com/open-telemetry/opentelemetry-java/issues/575#issuecomment-609169100

the points I've mentioned in the past about a base lib dependency having limitations are worse if no-one depends on it. right now, at least grpc does. get me?

[trask]

@adriancole The initial source tree was mostly done by @bogdandrutu, I believe it is just gRPC minus cancellations, did you see some other decisions that you wanted to discuss? @bogdandrutu is discussing alignment with the gRPC team.

[adriancole]

I'll try to put this a different way. the code seems lifted from grpc, which itself had years of decisions made. Which decisions are retained into this codebase are themselves decisions.

This project forked opencensus, but copied in its source (allowing review) piecemeal. For example, the "initial commit" here was just a license file. it would seem a similar policy would make similar sense for propagationio which if achieves its goal has an even wider scope than this project.

Also, this is far bigger than just me and bogdan. I like that you are keeping me up to date, but how could any diverse review happen if commits are decided before anyone even knows the project exists?

making more sense now?

[trask]

That's fair.

So maybe it's premature to even have a github repo at this point then.

What I'd like to accomplish initially, is to see if this idea even has legs, which is part technical feasibility and part inter-project discussions (Bogdan has reached out to gRPC and I've reached out to Reactor).

Maybe it's better if I just put code under my own github account for these initial discussions?

How would you suggest going about this?

Thanks,
Trask

[adriancole]

What was done here is a good way to start https://github.com/open-telemetry/opentelemetry-java/issues/575#issuecomment-609169100 You'll notice in the associated PR there's a bunch of chatter. You can ping those in your initial commit PR. I think it would be better to use wherever the repo intends to be as that keeps comment history. That said repos can be moved.

[trask]

Sounds good.

I will reset the repo tomorrow.

Do you think PR #1 should be only what's currently in the initial commit, or ok to also include what's currently in PR #1, in order to give reviewers a better picture of the proposed project direction?

[adriancole]

my suggestion would be that the pr should include only the most important core functionality (not the project setup which can be a scaffolding commit that's more bikeshed in nature)

There should be a rationale file to explain why the choices are there (helps conserve time of others and also with ESL). the test tree could have a features directory showing why certain things exist. If they don't have a why test, probably not worth putting in the codebase.

Also, I'd hesitate about adding all utility functions (like runnable wrappers) in the first PR for the same reason. Ex executor decorators etc unless they are actually powering something needed in the first feedback round.

In the PR description you can say that for mutual energy conservation, this won't include all features, and maybe just note the api features that are planned to be there by the end of it.

For example, a core assumption is that other libraries might use this. You could add integrations for some common things not just grpc ex
https://github.com/openzipkin/brave/blob/master/brave/src/test/java/brave/features/finagle_context/FinagleContextInteropTest.java

I suspect there are likely less than 5 total apis you'd want on that type anyway.

ex in brave, there's only

  public abstract Scope newScope(@Nullable TraceContext currentSpan);

  public interface ScopeDecorator {
    Scope decorateScope(@Nullable TraceContext currentSpan, Scope scope);
  }

everything else is compositional on those.

hope this helps

[trask]

Still trying here 😆. Check out #1189.

[Oberon00]

I think this issue is more serious than I initially thought. See my comment at https://github.com/open-telemetry/opentelemetry-java/pull/1281#discussion_r431768732:

I don't think we want enforce versions for all grpc components for all artifacts. But probably this is a (serious) issue that will only be resolved with #575. That is, even though we "only" depend on grpc-context, we force every user of the API to only use grpc versions (including other artifacts) that can work together with our grpc-context version. BTW, if grpc also depends on the proposed "propagation.io" context, we will still have the same problem.

[trask]

Interestingly, the specification is pretty clear on what should be done:

In the cases where an extremely clear, pre-existing option is not available, OpenTelemetry MUST provide its own Context implementation.

[jkwatson]

I have mostly not voiced much opinion on this topic because I have felt that I didn't know enough to have a firm opinion. However, at this point, my opinion is this:

We should have a java-interface-based (non-static-method-oriented) context API. I think it should probably be done outside the project, like the java-context prototype and consumed by this project.

I think it would be a mistake to bake in static-method-only APIs, as they are the most limiting choice you can make and force every user of the API to conform to that model.

There is no answer here that will make everyone happy, or satisfy every use-case, so we should choose the approach that is the most modern java-idiomatic, and that is, without any question, to base the API on a java interface, and thus support any dependency-injection use-cases. grpc-context, or something shaped the same way might be the "easiest" choice, but I don't think it's the right choice for this API.

[yurishkuro]

I agree, in principle. However, the implication here is that methods like tracer.startSpan(), which is currently documented as "make the new span a child of active span if the latter exists", become impossible unless the context object is passed in as an argument.

[Oberon00]

The context manager could be stored by the TracerProvider.

[yurishkuro]

@Oberon00 it would still limit the implementation to some form of thread-local storage, which is especially annoying to teams who make a choice to use explicit context propagation.

[jkwatson]

Yes, as @Oberon00 says, if we make the storage explicit, rather than implicit, then the SDK can be configured with the storage, and the SDK Tracer Provider can have the storage and provide access to the context for the tracer, as appropriate.

I'm not suggesting that we don't have storage somewhere; just that we eliminate the need to ever have to call Context.current() to get the current context, and instead make it state held (probably) by the tracer/provider.

[carlosalberto]

The extra layer sounds like something nice to play with, but honestly I don't see any general-purpose library that support this, other than grpc-context.

Unless we want to de-facto do this to specifically help Reactor/Armeria applications hook up their context layer when their, uh, applications run. That, or have Zipkin or other APM vendor implement their very own SDK, but I don't imagine this being the case in the near future?

[mattnworb]

Hi, as a user of both gRPC and OpenCensus, there is some level of convenience in having a single "Context" class/instance I can use to pass the (OpenCensus) trace and (gRPC) request context explicitly from method to method.

I certainly understand that being exposed to a class named io.grpc.Context is confusing for applications that do not use gRPC, but if OpenTelemetry were to create its own Context-like class with a different name, for codebases already "bought in" to io.grpc.Context then having a second Context class to explicitly propagate from method-to-method and thread-to-thread would add friction as well. For "microservice"-like Java applications that use multiple threads to handle processing of a request (at least those that are exposing gRPC servers), I have found it unworkable to use implicit propagation with ThreadLocals, so a second OpenTelemetry-specific Context class would have to be passed explicitly as well (or would force developers to define their own value class holding multiple Context implementations to have a single variable to pass).

I can't help but notice that opentelemetry-go can easily refer to context.Context from Go's standard library in its Tracer API, which I think speaks to the usefulness of having a commonly used Context implementation/interface across libraries and RPC frameworks. It'd be awesome if OpenTelemetry, gRPC, and other projects could find a way to share a common Context definition with a minimal API surface (so that it changes rarely), since it seems unlikely for anything to be added to the JDK.

[FroMage]

Hi,

I work on MicroProfile Context Propagation (https://download.eclipse.org/microprofile/microprofile-context-propagation-1.0/microprofile-context-propagation.html) and noticed this issue which touches on a lot of topics we are concerned with, so I thought perhaps I can shed some light on what we do about context propagation.

First, unlike gRPC Context, MicroProfile Context Propagation (MP-CP for short) does not provide storage: it doesn't give you a context class where you can store your context info (type-safe or not). It doesn't do this because it's trivial to implement using a class with a ThreadLocal (or equivalent), a few methods for get/set and lifecycle. And also because a lot of existing libraries have existing context classes that they want to keep, or their lifecycle is complicated, or they have custom requirements for serialising it.

What MP-CP does however, is propagate any number of pluggable contexts without caring how they're defined by the frameworks that use them. In short, if your framework has a context class, you only have to implement a ThreadContextProvider interface and provide it with methods for:

• capturing a context (getting the ThreadLocal that holds your context for example)
• restoring a context (setting the ThreadLocal that holds your context for example)
• clearing a context (setting the ThreadLocal that holds your context to null for example)

And once you do that, all applications that use context propagation will have your context propagated.

The propagation is really the hard part, because many applications support either async parallel methods that spawn a request (if your framework is request-based) into offshoots of the request processing that run on separate threads or executors, none of which would normally see the ThreadLocal where frameworks store their contexts. Or in the case of reactive programming, there's going to be a lot of callbacks executed on whatever current thread calls them, or executors, which again in both cases will have no ThreadLocal or worse: the wrong one.

What MP-CP does is that it allows applications that want contexts to be propagated to wrap their callbacks into ones that capture the current context on creation, and restore it for the duration of its execution. There's context propagation for common callbacks such as Runnable, Function, Supplier but also CompletionStage and CompletableFuture.

We also have a plugin mechanism which allows us to hook automatic context propagation for reactive libraries such as RxJava or Mutiny, which means the user doesn't have to change their code at all to get context propagation automatically and intuitively.

As far as I'm aware, MP-CP is the only solution which provides pluggable context propagation without forcing you to handle the propagation yourself, or to change your context storage solution, or to change your underlying reactive API.

Given your requirements, I think you could just define your own storage class, which can have type-safe keys if you need that, and custom serialisation if you send this over the wire, and as long as you allow access to getting and setting it, you can (or someone else can) implement a plugin for MP-CP that will make it propagated for everyone using MP-CP for propagation.

ATM MP-CP only defines interfaces and depends only on MP-Config so it's a fairly small dependency, and you don't need to depend on an implementation (such as SmallRye Context Propagation). You could split your ThreadContextProvider into a new module just containing that class if you want to avoid the dependency, and people who want propagation of your context would import it. Or you can put that class in your base module and everyone using context propagation would automatically get your context propagated when they import your module.

It's not clear to me whether you need an implementation of context propagation in your module, however, given that you currently use gRPC context which doesn't do propagation (in process), so I think this is not a requirement for you ATM.

Hope that helps, and let me know if you have any question. Cheers.

[iNikem]

@FroMage This is very interesting for us in auto instrumentation for Java. Can you please take a look at this use-case? How does MP-CP handle such "run-away" contexts?

[jkwatson]

@FroMage Super interesting. I'm going to set aside from time to read the spec document. Thanks for linking this here!

[sjoerdtalsma]

@FroMage thanks for the link. It sounds like a perfect substitute for my hand-rolled api. I'll definitely take a look at it.

[FroMage]

Can you please take a look at this use-case? How does MP-CP handle such "run-away" contexts?

You can either inject a ManagedExecutor which clears all contexts for async actions if the context is meant to be closed before they terminate, or if the context is meant to be kept open and used until all such async actions are completed, then it's up to your application to keep track of when those async actions terminate before you can close your context.

[jkwatson]

@FroMage Is Microprofile Context Propagation locked to java 8+, or could it be used with java 7?

[carlosalberto]

Assigning this to @malafeev as he will do an initial digging & comparison of MicroProfile and grpc's Context.

[malafeev]

Java 7 is not supported by MP-CP API. It's based on Java 8 features.
When I tried to compile

 ManagedExecutor executor = ManagedExecutor.builder()
        .maxAsync(5)
        .propagated(ThreadContext.CDI, ThreadContext.APPLICATION)
        .build();

    ThreadContext threadContext = ThreadContext.builder()
        .propagated(ThreadContext.SECURITY)
        .build();

I got

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project test: Compilation failure: Compilation failure: 
[ERROR] /Users/malafes/projects/test/src/main/java/Main.java:[8,47] static interface method invocations are not supported in -source 1.7
[ERROR]   (use -source 8 or higher to enable static interface method invocations)
[ERROR] /Users/malafes/projects/test/src/main/java/Main.java:[13,48] static interface method invocations are not supported in -source 1.7
[ERROR]   (use -source 8 or higher to enable static interface method invocations)

[FroMage]

Yeah, it's JDK8+

[adriancole]

I would summarize this as another key consumer/integration besides grpc-context. There is also https://github.com/alibaba/transmittable-thread-local which focuses on some of this even if we formerly mostly discussed RPC and reactive.

The main idea here was keeping the deps clear and knowing that while solving for gRPC is important, so is making other things possible, which share different dependency characteristics, etc.

[malafeev]

from my MP CP review:

• MP CP is a set of interfaces to implement.
• MP CP is not a solution which we can take and use without doing own implementation of interfaces.
• MP CP is coupled to Executors. Threads should be managed by container...
• MP CP is java 8+. It’s based on java 8 concurrency api: completable future is a first citizen.
• MP CP must to be compatible with Java EE Concurrency model/api, therefore it may have unnecessary functionality.
• I was not able to create an example to propagate value like a string from one thread to another thread reading documentation. So it’s still a black box for me.

[FroMage]

MP CP is not a solution which we can take and use without doing own implementation of interfaces.

Well, there's SmallRye-ContextPropagation which implements it that you can use. I don't see the point in re-implementing it.

MP CP is coupled to Executors. Threads should be managed by container...

That's only the case for ManagedExecutor but not for ThreadContext which is what you will want to use.

MP CP is java 8+. It’s based on java 8 concurrency api: completable future is a first citizen.

Yes, that's the case.

MP CP must to be compatible with Java EE Concurrency model/api, therefore it may have unnecessary functionality.

This is only true of ManagedExecutor: a single class.

I was not able to create an example to propagate value like a string from one thread to another thread reading documentation. So it’s still a black box for me.

Do you want me to provide a complete example here?

[malafeev]

Do you want me to provide a complete example here?

@FroMage it would be nice, thanks

[FroMage]

So, there's your test class:

package whatever;

import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

import org.eclipse.microprofile.context.ThreadContext;
import org.eclipse.microprofile.context.spi.ContextManager;
import org.eclipse.microprofile.context.spi.ContextManagerProvider;
import org.junit.Assert;
import org.junit.Test;

public class DemoTest {
    @Test
    public void test() throws InterruptedException, ExecutionException {
        ContextManager contextManager = ContextManagerProvider.instance().getContextManager();
        ThreadContext threadContext = contextManager.newThreadContextBuilder().build();

        ExecutorService executorService = Executors.newFixedThreadPool(2);
        MyContext.clear();
        // start with no context
        Assert.assertNull(MyContext.get());

        // verify that new threads also get no context
        executorService.submit(() -> {
            Assert.assertNull(MyContext.get());
        }).get();

        // now set the context
        MyContext ctx = new MyContext();
        MyContext.set(ctx);
        // verify that we have a context
        Assert.assertEquals(ctx, MyContext.get());

        // verify that new threads still get no context
        executorService.submit(() -> {
            Assert.assertNull(MyContext.get());
        }).get();

        // verify that new threads with context propagation do get context
        executorService.submit(threadContext.contextualRunnable(() -> {
            Assert.assertEquals(ctx, MyContext.get());
        })).get();
    }
}

And here's your context class:

package whatever;

public class MyContext {

    private static ThreadLocal<MyContext> context = new ThreadLocal<MyContext>();

    public static void init() {
        context.set(new MyContext());
    }

    public static void clear() {
        context.remove();
    }

    public static MyContext get() {
        return context.get();
    }

    public static void set(MyContext newContext) {
        context.set(newContext);
    }

// put your context properties here
    private String reqId;

    public void set(String reqId) {
        this.reqId = reqId;
    }

    public String getReqId() {
        return reqId;
    }
}

This is typically where you'd put whatever context values you need.

You'll need to have this src/main/resources/META-INF/services/org.eclipse.microprofile.context.spi.ThreadContextProvider file too:

whatever.MyThreadContextProvider

You'll want to use SmallRye-CP:

        <dependency>
            <groupId>io.smallrye</groupId>
            <artifactId>smallrye-context-propagation</artifactId>
            <version>1.0.16</version>
        </dependency>

That's all. Now you can both propagate any number of contexts, and make sure that your context will be propagated to any CP user.

Does that help?

[jkwatson]

In the 9/15[16] java meeting, it was agreed that @anuraaga will create a new module and start building our own context implementation. Thanks @anuraaga for spearheading this work!