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

Pass observation to tasks #5491

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaldo
Copy link

As developer, I would like parallelize my code and observe subtasks (https://stackoverflow.com/questions/78746378/spring-boot-3-micrometer-tracing-in-mdc/78765658#78765658 https://stackoverflow.com/questions/78889603/traceid-propagation-to-virtual-thread).

I found solution with ContextExecutorService, but according to discussion micrometer-metrics/context-propagation#295, ContextExecutorService is low level API, which should not be used by application code.

Observation is enough high level to be used by application code.

As developer, I would like parallelize my code and observe subtasks (https://stackoverflow.com/questions/78746378/spring-boot-3-micrometer-tracing-in-mdc/78765658#78765658 https://stackoverflow.com/questions/78889603/traceid-propagation-to-virtual-thread).

I found solution with `ContextExecutorService`, but according to discussion micrometer-metrics/context-propagation#295, ContextExecutorService is low level API, which should not be used by application code.

`Observation` is enough high level to be used by application code.
@chemicL
Copy link

chemicL commented Oct 2, 2024

As explained in micrometer-metrics/context-propagation#295 (comment) Observation is a specific user of the context-propagation library and should not delegate to its generic capabilities for the purpose of propagating a specific state. I believe this PR should be discarded.

While context-propagation is a lower level API it needs to be used in custom cases when users want to propagate context to ExecutorService instances which they are in control of.

@shakuzen
Copy link
Member

shakuzen commented Oct 4, 2024

At the risk of saying the same thing @chemicL already said but in a different way: Observation does not have a hard dependency on context-propagation, so adding a method to it that uses context-propagation is a non-starter, I think. In the micrometer-observation module, it is only the ObservationThreadLocalAccessor class that depends on context-propagation.

Maybe we could add something to our Micrometer Observation documentation to talk about using the context-propagation library as a solution for propagating Observation across thread switches, such as when using an ExecutorService.

@jonatan-ivanov
Copy link
Member

Tests here might help, particularly this one:

void start_thenStopOnChildThread() throws InterruptedException, ExecutionException {
ExecutorService executor = Executors.newSingleThreadExecutor();
Observation observation = Observation.createNotStarted("test.observation", registry);
assertThat(registry.getCurrentObservation()).isNull();
executor.submit(() -> {
try (Observation.Scope scope = observation.openScope()) {
assertThat(registry.getCurrentObservation()).isSameAs(observation);
}
observation.stop();
}).get();
assertThat(registry.getCurrentObservation()).isNull();
}

You should be able to open a scope for your parent Observation inside of the task and then create a child observation (and a child scope). The limiting thing is that the parent observation should be "visible" from the code of the task but in your use-case this might not be an issue.

Something like this:

ExecutorService executor = Executors.newSingleThreadExecutor(); 
  
Observation parent = Observation.start("parent", registry); 
executor.submit(() -> {
    try (Observation.Scope ignored = parent.openScope()) { 
        // create a child observation here and do what you want
    }          
});
// collect futures and wait for every future to finish their job
parent.stop(); 

This should work with parallel streams too.

@michaldo
Copy link
Author

michaldo commented Oct 8, 2024

@jonatan-ivanov, let me recap

  1. I meet problem with passing observation to tasks and I applied similar solution as you proposed. Let's name in "manual solution"
  2. Then igor.zh pointed me altermative solution, based on ContextExecutorService. Let's name it "low level solution"
  3. As low_level solution requires complex setup, not adequtate to simple expected goal, I provided this PR, high level solution

So question is simple: what is correct solution to solve my problem: passing observation to tasks.

Manual solution for typical problem looks as reinventing wheel
Low_level solution requires deep knowledge how library works internally
High level solution is probably too rough

Update

Maybe my question could be much simple:

I would like pass observation to task. I would like add a static method to Observation API. Should implementation use ContextExecutorService or not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants