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

Micrometer Observation Support #17436

Open
marcingrzejszczak opened this issue Aug 14, 2023 · 11 comments · May be fixed by #17494
Open

Micrometer Observation Support #17436

marcingrzejszczak opened this issue Aug 14, 2023 · 11 comments · May be fixed by #17494

Comments

@marcingrzejszczak
Copy link
Contributor

Describe your motivation

I'm a co-maintainer of Spring Cloud Sleuth and Micrometer projects (together with @shakuzen and @jonatan-ivanov).

Micrometer Observation is part of the Micrometer 1.10 release and Micrometer Tracing gives support for Tracing capabilities. The idea of Micrometer Observation is that you instrument code once but you get multiple benefits out of it - e.g. you can get tracing, metrics, logging or whatever you see fit).

I was curious if there's interest in adding Micrometer Observation support so that except for metrics, spans could be created and tracing context propagation could happen too. Via Micrometer Tracing one can use OpenTelemetry or OpenZipkin Brave Tracer, but with the handler mechanism the possibilities are endless :)

Describe the solution you'd like

We can add a PR to instrument the RPC communication of Vaadin.

Additional context

Micrometer Observation Documentation

@mshabarov
Copy link
Contributor

Hello @marcingrzejszczak and thank you for creating this enhancement!
This sounds for me (and I bet for all the Vaadin team) more than interesting. We consider the "observability" as one of the most valuable corners of business applications. As a product owner of Vaadin Flow, I'd appreciate any work on this topic. If you have any thought or even a suggestions about actual implementation towards integrating Micrometer Observation into Vaadin, please share it in the pull request https://github.com/vaadin/flow/compare . Vaadin Flow team is happy to review, test and help driving this feature further.

@mshabarov
Copy link
Contributor

please share it in the pull request https://github.com/vaadin/flow/compare

@marcingrzejszczak we discussed Micrometer integration internally with the team. We are considering adding this support to our commercial Observability Kit, and make a research by ourselves first. Thus, I would ask you to hold your contribution for a while. We'll get back to you once we get a vision of the solution and whether Micrometer Observation can be added into Vaadin Flow core.

@mshabarov mshabarov moved this from Inbox (needs triage) to Maybe in Vaadin Flow enhancements backlog (Vaadin 10+) Aug 15, 2023
@mshabarov mshabarov moved this from Product backlog to Parking lot - under consideration in OLD Vaadin Flow ongoing work (Vaadin 10+) Aug 15, 2023
@mshabarov
Copy link
Contributor

@marcingrzejszczak we decided to proceed with Micrometer and Vaadin integration and to try instrument the Vaadin Flow core/RPC communication. Please assign me to the pull request and/or ping me whenever you need a help or assistance. Thank you!

@mshabarov mshabarov moved this from Maybe to Next for Dev. Team in Vaadin Flow enhancements backlog (Vaadin 10+) Aug 17, 2023
@mshabarov mshabarov moved this from Parking lot - under consideration to Product backlog in OLD Vaadin Flow ongoing work (Vaadin 10+) Aug 17, 2023
@marcingrzejszczak
Copy link
Contributor Author

Hey, thanks!

I've started some initial analysis and would definitely could use with your help.

Do I understand correctly that wrapping a VaadinService's requestStart and requestEnd would take care of all positive scenarios (non-error ones) of creating observability on all inbound requests? It would be great if handleExceptionDuringRequest was at least protected so that we can handle also the error scenario... I was looking at RequestHandler interface but that's not a good abstraction because it doesn't give you an around wrapper just before / after. I would need to create 2 implementations, one that is called before and creates an observation and one that is created at the end that would close the observation and all scopes and that seems too artificial.

If my analysis is not correct can you please tell me what should be the point of entry to all requests Vaadin and where I can find the points of egress, so all clients that Vaadin uses to send requests over the wire.

Another question is what key / value pairs would be interesting for users as high cardinality (can have a lot of different values e.g. use name) and low cardinality (not a lot of values e.g. http method) tags?

@marcingrzejszczak
Copy link
Contributor Author

@mshabarov do you want me to mention you when I have questions or just comment here and wait for your answer?

@mshabarov
Copy link
Contributor

I get the notifications anyway :)
Both are fine for me. I saw your questions, need to dig into it a bit more. Will get back to you with a full answer.

@mshabarov
Copy link
Contributor

mshabarov commented Aug 24, 2023

Do I understand correctly that wrapping a VaadinService's requestStart and requestEnd would take care of all positive scenarios (non-error ones) of creating observability on all inbound requests?

requestStart is almost the first thing Vaadin invokes after servlet gets a http request. Note that static resources are handled before it, so this hook wouldn't take it into account, a custom VaadinServlet might be needed to track static resources requests if needed.
requestEnd is literally the last hook Vaadin calls just after clearing thread locals related to this request and unlocking the session.
Thus, I think this is a good place to track the overall request lifecycle scope and put observations. If you needed to track more precise steps in the request processing, you might want to wrap the existing RequestHandlers from a list defined in VaadinService::createRequestHandlers.

It would be great if handleExceptionDuringRequest was at least protected so that we can handle also the error scenario

For this purpose, I'd propose to create a new protected method in VaadinService with empty body and call it inside handleExceptionDuringRequest. This method can be overridden to add the observability.

Another question is what key / value pairs would be interesting for users as high cardinality (can have a lot of different values e.g. use name) and low cardinality (not a lot of values e.g. http method) tags?

Good questions, for this I need a bit more time to answer. Will be back soon.

@marcingrzejszczak
Copy link
Contributor Author

Another question - are you willing to make Micrometer Observation a compile-time dependency in Flow or do you want it to be optional? I have an idea on how to implement the feature but the solution depends on the answer to the question. From my perspective obviously it would be easier if it is a mandatory dependency then it will be easier to not only add observations but also other metrics we can add later.

@marcingrzejszczak marcingrzejszczak linked a pull request Aug 24, 2023 that will close this issue
9 tasks
@marcingrzejszczak
Copy link
Contributor Author

In the meantime I've created a draft PR to show how I think this could be done #17494 . I took it out for a spin with the skeleton example with Spring Boot and it seems to be working locally (just FYI for all of that to work I'm using a branch of Micrometer 1.12.0 that has Jakarta instrumentation in it so the PR will be permanently broken until we merge the feature). This is what I got in Zipkin

image

@mshabarov
Copy link
Contributor

Another question is what key / value pairs would be interesting for users as high cardinality (can have a lot of different values e.g. use name) and low cardinality (not a lot of values e.g. http method) tags?

This is a list of metrics, including Vaadin-specific ones, that our Observability Kit exposes, this can be used as a reference https://vaadin.com/docs/latest/tools/observability/reference#metrics to have insights on what users might want to monitor. About cardinality: within Vaadin + Micrometer integration, do we need to handle them differently or somehow mark them differently?

@marcingrzejszczak
Copy link
Contributor Author

marcingrzejszczak commented Aug 25, 2023

This is a list of metrics, including Vaadin-specific ones, that our Observability Kit exposes, this can be used as a reference https://vaadin.com/docs/latest/tools/observability/reference#metrics to have insights on what users might want to monitor. About cardinality: within Vaadin + Micrometer integration, do we need to handle them differently or somehow mark them differently?

No that list is fine. I'll have a look at that later.

So I'm splitting the PR into 2. One will introduce VaadinFilters the other will add instrumentation.

Once you know what are the egress points out of Vaadin don't hesitate to ping me and I'll look if any instrumentation wouldn't be beneficial there (unless there's already sth there).

marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Aug 25, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Aug 25, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
@mshabarov mshabarov moved this from Product backlog to In progress in OLD Vaadin Flow ongoing work (Vaadin 10+) Aug 30, 2023
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Aug 31, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 4, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 12, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 12, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 15, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 15, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 15, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 25, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
marcingrzejszczak added a commit to marcingrzejszczak/flow that referenced this issue Sep 25, 2023
VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
mshabarov added a commit that referenced this issue Sep 26, 2023
VaadinRequestInterceptor simulates an around aspect around processing of a request.

Related-to #17436

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🔎Iteration reviews
Development

Successfully merging a pull request may close this issue.

2 participants