vaadin / observability-kit

Other
5 stars 2 forks source link

feat: add Push instrumentation #75

Closed caalador closed 2 years ago

caalador commented 2 years ago

Add instrumentation for push.

Closes #69

sissbruecker commented 2 years ago

When testing this with the business-starter, I can see two spans pop up in a regular interval:

Bildschirmfoto 2022-08-31 um 11 36 48

The first one is the request, the other one is created by the instrumentation and doesn't seem to run within the request. Is this intentional?

Also I tested this with an app downloaded from start.vaadin.com (running in production mode), here the only the Push: onRequest span shows up once, and then nothing else, even though push works fine 🤷‍♂️.

Maybe it would be more interesting to instrument UI.access rather than the individual handlers, although if requests already create a root span, like with the business starter, then it would be good to mark them as push requests somehow.

caalador commented 2 years ago

The empty span if you look into it is most likely the 101 Switching Protocols request which is handled by the server without coming to application code at all.

sissbruecker commented 2 years ago

Thanks, I guess we can't do anything about that one.

Still I'm not sure what the purpose of this instrumentation is. I just added some push implementation that does some SQL queries that show up as root spans, instead of getting nested under a push span. For push, the only expectation that I had was that I can see that some operation in my app was run as part of push, rather than a frontend request. Is that possible somehow?

caalador commented 2 years ago

I would perhaps leave this to wait for now as the idea was to get the unknown /* and / handled, but as they are mostly only 101 requests it didn't help that much. I would expect onMessage to be called when a push is sent from the server, but with all the other issues we have I didn't investigate further with changes to an application.

caalador commented 2 years ago

Closing PR as there is nothing that seems to help with push handling.