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

[Enhancement request] micrometer observability for the new StructuredTaskScope api #5761

Open
patpatpat123 opened this issue Jan 1, 2025 · 5 comments
Labels
waiting for feedback We need additional information before we can continue

Comments

@patpatpat123
Copy link

patpatpat123 commented Jan 1, 2025

Please describe the feature request.
As of today, following the doc: https://docs.micrometer.io/micrometer/reference/observation/instrumenting.html#instrumentation_of_thread_switching_components

We can find an example with

ExecutorService executor = Executors.newCachedThreadPool();
[...]
return ContextExecutorService.wrap(executor).submit(()

(small side requests here, would it be possible to enhance this current example with
a - virtual thread
b - the autoclosable try with resource
c - wrap is deprecated, but there is no mention of what should be the replacement
)

However, the doc seems to be missing, as well as the ContextExecutorService.wrap api seems to be missing support for the new StructuredTaskScope<> api.

Rationale
As shown in many official Java / Oracle videos and official documentation (here is one for example)
https://www.youtube.com/watch?v=zPhkg8dYysY

Screenshot 2025-01-06 at 10 28 59

Java now offers the new StructuredTaskScope<> api.
However, the structured concurrent tasks cannot be observed with micrometer.
For instance, ContextExecutorService.wrap cannot take the structured task scope.

It would be great if micrometer could offer a simple and friendly API to support observability for tasks spawning from structured concurrency (showing the parallelism, errors, structures, etc...)

Thank you

@shakuzen
Copy link
Member

shakuzen commented Jan 6, 2025

Structured Concurrency is still a Preview Feature, even in the upcoming JDK 24 - see JEP 499.

For instance, ContextExecutorService.wrap cannot take the structured task scope.

As its name implies, ContextExecutorService is for things of type ExecutorService, which StructuredTaskScope is not. Any support for StructuredTaskScope I would expect to be separate from ContextExecutorService.

StructuredTaskScope deals with ScopedValues (another Preview Feature - see JEP 487) which we opened micrometer-metrics/context-propagation#108 to explore. That hasn't yet resulted in any changes to the library so far. I think we need more investigation and proposals to see what changes make sense and how they would (or would not) be compatible with existing API. If someone wants to help with this, it may help move things along.

I wonder if this enhancement request belongs in the Micrometer repo at all. I suppose if we want to instrument all forks in a scope (I'm not sure we would want that - it may be better that users decide specifically what they want instrumented), there may be something to do here. But probably figuring out context propagation is also needed regardless.
/cc @chemicL

@patpatpat123
Copy link
Author

Thank you, @shakuzen, for the clear writing.

You are correct the feature is still in preview even for JDK 24 which is around the corner.
With that said, there is a lot of investment in this from Java, and even with changes, the feature might not completely go away.

Would it be possible to leave this enhancement request open and gather some more discussions here about the integration between StructureRaskScope and micrometer while the feature is getting finalized?

@shakuzen
Copy link
Member

shakuzen commented Jan 6, 2025

Would it be possible to leave this enhancement request open and gather some more discussions here about the integration between StructureRaskScope and micrometer while the feature is getting finalized?

Yes, I think we'd like to hear more feedback about what instrumentation users like yourself would like to see. Maybe if you can give some sample code and explain which parts you want instrumented and how (feel free to propose some API that could exist in the future).

@marcingrzejszczak marcingrzejszczak added waiting for feedback We need additional information before we can continue and removed waiting-for-triage labels Jan 9, 2025
@patpatpat123
Copy link
Author

patpatpat123 commented Jan 9, 2025

Thank you for all the responses.

Just trying to kick off things:

In this very simple sample:

    @GetMapping("/test")
    String hello() {
        try (var scope = new StructuredTaskScope<>()) {
            Observation parent = Observation.createNotStarted("parent", registry); // Trouble finding what to do with this parent
            StructuredTaskScope.Subtask<String> imagesSubtask = scope.fork(() -> someService.readImages());
            StructuredTaskScope.Subtask<String> linksSubtasks = scope.fork(() -> someService.readLinks());
            scope.join();
            return imagesSubtask.get() + "good " + linksSubtasks.get();
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }
    }

Which represents the example from Jose Paumard.

On a tracing point of view,:
Expected: it would be good if we could see the parallel traces execution of the above.
Something like this:
Screen Shot 2025-01-09 at 20 58 50

However, as of now, the traces are scattered, they are on their own.

Screen Shot 2025-01-09 at 20 59 07

And it seems there are no ways to "correlate" those as of now.
P.S. this example is showing some kind of network call, but could be applicable for DB call, message queue systems etc

On a metrics point of view:
Would be great if there could be some metrics to show the time of the parallelism.
Also, maybe some kind of counts of failure/successes of the forks

@marcingrzejszczak
Copy link
Contributor

Shouldn't you start the parent observation, put it in scope and wrap the runnables for both tasks?

sth like this?

    @GetMapping("/test")
    String hello() {
        try (var scope = new StructuredTaskScope<>()) {
           return Observation.createNotStarted("parent", registry)
           .observe( () -> {
              // Capture all thread local values
              ContextSnapshot snapshot = ContextSnapshotFactory.builder().contextRegistry(ContextRegistry.getInstance()).build().captureAll();
                          
              StructuredTaskScope.Subtask<String> imagesSubtask = scope.fork(snapshot.wrap(() -> someService.readImages())); // wrap the runnables so that captured values will be restored within the new thread
              StructuredTaskScope.Subtask<String> linksSubtasks = scope.fork(snapshot.wrap(() -> someService.readLinks()));  // wrap the runnables so that captured values will be restored within the new thread
              scope.join();
              return imagesSubtask.get() + "good " + linksSubtasks.get();
            });
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }
    }
    ```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback We need additional information before we can continue
Projects
None yet
Development

No branches or pull requests

3 participants