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

automatic tracing of ws #12

Open
vmarchaud opened this issue Jan 5, 2020 · 20 comments
Open

automatic tracing of ws #12

vmarchaud opened this issue Jan 5, 2020 · 20 comments

Comments

@vmarchaud
Copy link
Member

Is it applicable for Node or Browser or both
Node

Do you expect this plugin to be commonly used
Weekly Downloads: 10M (https://www.npmjs.com/package/ws)

What version of plugin are you interested in using
Versions: 5 - 7 (cover last 2 years of releases)

Additional context

  • Is there a reference you could point for the well-defined lifecycle methods

I believe only elastic implemented an intrumentation for this module (https://github.com/elastic/apm-agent-nodejs/blob/master/lib/instrumentation/modules/ws.js)

@obecny
Copy link
Member

obecny commented Jan 7, 2020

Should we also support web socket on web too then ?

@dyladan
Copy link
Member

dyladan commented Jan 7, 2020

Does websocket have a place to put the traceparent? afaik there is no way to add the tracing info without potentially modifying the user's payload?

@vmarchaud
Copy link
Member Author

@obecny I believe we should at some point but i would prefer to open a dedicated issue

@dyladan The websocket handshake is done in http, so we could use that i believe. It depend if we want one trace per connection or one trace per packet

@dyladan
Copy link
Member

dyladan commented Jan 8, 2020

One per connection is of dubious value... Most websocket use cases are long running and encompass many user actions.

@nstawski
Copy link
Contributor

nstawski commented Jan 8, 2020

I would love to take this task. Would take me some time to solve though.

@vmarchaud
Copy link
Member Author

@nstawski Feel free to ping me if you need any help or guidance on that, i've already looked up on how to implement that in the past.

@vmarchaud
Copy link
Member Author

Moving to contrib repo, also @nstawski did you started to work on that ?

@vmarchaud vmarchaud transferred this issue from open-telemetry/opentelemetry-js Apr 25, 2020
@dajulia3
Copy link

dajulia3 commented Feb 8, 2021

I for one would love this on both the node and browser side. I'd happily modify my payload with trace/span info to get tracing. Has anyone started to work on a solution for this? @vmarchaud

@dyladan
Copy link
Member

dyladan commented Feb 9, 2021

@dajulia3 I think nobody is currently working on it. @nstawski started working on it a while ago but she's not been involved in the project recently

@vmarchaud vmarchaud assigned dajulia3 and unassigned nstawski Mar 20, 2021
@vmarchaud
Copy link
Member Author

No sign from @nstawski, i'll assign it to you @dajulia3 then

@obecny
Copy link
Member

obecny commented Mar 22, 2021

@dajulia3 can you please confirm you are working on that ?

@dajulia3
Copy link

@vmarchaud @obecny I'm not actively working on instrumenting that library per se... I'm about to implement a bit of custom instrumentation for socket stream rpc calls which an older node app is using. If there are any relevant learnings from that I'll be happy to share them.

@airhorns
Copy link

airhorns commented May 9, 2021

I'd really like to make this a thing -- does anyone have any insight into if this is harder than it seems or half baked work we could adapt into a PR?

@vmarchaud
Copy link
Member Author

I'd really like to make this a thing -- does anyone have any insight into if this is harder than it seems or half baked work we could adapt into a PR?

I don't think this is specially hard but it depend on what you want to achieve:

  • making a new websocket connection create a span
  • sending a msg on a websocket connection create a span
  • do one of above while propagating the context (aka sending the span/trace id when sending a msg)

The last task is quite complicated because the ws protocol doesnt have any way to add metadata for a given message (as i said in one of my first messages is that you could do it for each connection but thats not for every use case).
I would suggest to start by creating a span for each message and see from there

@mottibec
Copy link
Contributor

mottibec commented Jun 3, 2021

we just released a socket.io plugin check it out

@roastedmonk
Copy link

we just released a socket.io plugin check it out

Will this work with ws package?

@mottibec
Copy link
Contributor

we just released a socket.io plugin check it out

Will this work with ws package?

no, only with the socket.io package

@airhorns
Copy link

airhorns commented Jan 18, 2022

I put together this instrumentation for the ws library here: https://www.npmjs.com/package/opentelemetry-instrumentation-ws , it traces the client side connection initiation and the server side, including the upgrade event and WebSocket initiation. We use the normal http propagators for http requests right now such that if you make a server to server ws connection propagation works great, but in the browser, I think we'll need something different because you can't set arbitrary headers on outgoing websocket HTTP requests. I think that will have to be done within whatever websocket protocol you're using as there's no other sidechannel I am aware of. We're gonna work on this for graphql-ws as well using it's ConnectionInit protocol message.

We've been running it in production for a few weeks now and it has helped us understand our websocket woes a lot better. We'd would love any feedback from some early adopters, especially around what should constitute a new trace vs a span within a long lived connection like a websocket. I think we'll love to upstream it to this contrib repo after building a bit more confidence it is the right shape! Source here: https://github.com/gadget-inc/opentelemetry-instrumentations/tree/main/packages/opentelemetry-instrumentation-ws

@muzammil360
Copy link

@airhorns nice work on opentelemetry-instrumentation-ws package. Any idea if something similar exists for websocket server initialized with Fastify?

I am running a fastify server and using @opentelemetry/instrumentation-fastify package to collect traces for http requests. But it doesn't seem to work with fastify websocket server.

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 4, 2024

Are there any plans to migrate the ws instrumentation to this repository?

RafalSumislawski added a commit to coralogix/opentelemetry-js-contrib-esbuild-node-plugin that referenced this issue Nov 20, 2024
This PR extends the logic of applying lambda handler instrumentation in two ways.
* Supporting handler defined in *.mjs files
* Adding additinal InstrumentationNodeModuleDefinition matching by filename, which turns out to be what is needed in a module environment with the import-in-the-middle-hook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants