Pass contextual information to the worker

[ggrossetie]

I was about to publish events using postMessage({ code: 'EVENT', name: 'eventName', args: [] }) in pino-socket but I don't know how to make sure that the event code is "supported" by thread-stream. In other words, it's only safe to post an EVENT message if we are using thread-stream > 2.1.x. If we are using older versions it will destroy the stream:

thread-stream/index.js

Line 167 in 7a395f7

destroy(stream, new Error('this should not happen: ' + msg.code))

Anyway, I was wondering if it would make sense to pass contextual information, such as the version (or a list of capabilities? or a "communication protocol" version) to the worker?

It would allow the transport/worker to communicate with the thread stream "safely" knowing which messages can be sent.

I'm also open to ideas on how to solve this issue.

Follow-up: #97

[jsumners]

Don't you control the application's dependencies? I think it would be simpler to set a minimum install version of the dependency in your application such that it supports the features you need.

[mcollina]

That'd be handy. Would you like to send a PR? You can include it in

thread-stream/index.js

Line 52 in 7135302

workerData: {

.

[ggrossetie]

Don't you control the application's dependencies? I think it would be simpler to set a minimum install version of the dependency in your application such that it supports the features you need.

Yes but thread-stream is a transitive dependency. Most users will only install a specific version of pino and pino-socket. Arguably, we could mention (in the README) that pino-socket version Y is only compatible with pino X (which brings thread-stream version Z) but that's one more thing to check and my guess is that quite a few users are going to fall for it.

It won't matter that much if EVENT messages were ignored in previous versions but in this case it will destroy the stream.

That'd be handy. Would you like to send a PR? You can include it in

Sure!
I believe that passing thread-stream version is probably enough. Using an arbitrary communication protocol version might be confusing and using a list of capabilities is probably a bit too much.

So I was thinking something like:

workerData = {
  '$context': {
    threadStreamVersion: version
  },
  ...workerData
}

I'm using a special prefix to avoid conflicts. I don't have any preference about the prefix character though.

[jsumners]

Yes but thread-stream is a transitive dependency. Most users will only install a specific version of pino and pino-socket.

Which is all handled by the semver ranges specified in those direct dependencies.

🤷‍♂️

[ggrossetie]

Which is all handled by the semver ranges specified in those direct dependencies.

pino-socket does not have a dependency on thread-stream. In this case, you cannot prevent a user from using incompatible versions... unless I'm missing something?

I don't think we should add an explicit dependency on pino-socket since, as far as I know, we can use pino-socket without thread-stream.

So, one way would be to explicitly add a dependency on thread-stream >= 2.1.x (in the application dependencies) since pino accepts version >= 2.0.0 < 3.0. But then again it falls on users to explicitly manage the version of thread-stream.

[jsumners]

I do not understand the setup. Whatever application is using thread-stream should be the one that has the correct qualifiers.

[ggrossetie]

Pino is using thread-stream and declares version ^2.0.0.
pino-socket transport might be called as a worker through thread-stream and might send messages to the parent thread using parentPort.
There's no direct link between pino-socket and thread-stream other than a "communication protocol":

thread-stream/index.js

Lines 152 to 175 in 7135302

	switch (msg.code) {
	case 'READY':
	// Replace the FakeWeakRef with a
	// proper one.
	this.stream = new WeakRef(stream)
	stream.flush(() => {
	stream[kImpl].ready = true
	stream.emit('ready')
	})
	break
	case 'ERROR':
	destroy(stream, msg.err)
	break
	case 'EVENT':
	if (Array.isArray(msg.args)) {
	stream.emit(msg.name, ...msg.args)
	} else {
	stream.emit(msg.name, msg.args)
	}
	break
	default:
	destroy(stream, new Error('this should not happen: ' + msg.code))
	}

This "protocol" changed in version 2.1.0 and now it allows messages with the code EVENT. Previously, a message with the code EVENT was considered as unexpected and as a result the stream was destroyed.

This is not a major version because the protocol is backward compatible but if a transport sends a message with the code EVENT when using thread-stream< 2.1, the stream will be destroyed!
That's why we need to make sure that the communication protocol supports this new code.

thread-stream is not a user facing dependency and as mentioned above pino-socket does not have a direct dependency on it.

Does it make sense?

Are you suggesting that user applications should manage pino and thread-stream versions independently while making sure that pino version X is compatible with thread-stream version Y?
Do you think it's a big concern to add contextual information to the worker?

[jsumners]

My concern is adding more features to maintain.

Are you saying you want to modify the pino-socket code to send a message through a message channel?

[ggrossetie]

My concern is adding more features to maintain.
Are you saying you want to modify the pino-socket code to send a message through a message channel?

Yes, but it's not really a new feature, it's an existing feature that does not work when the transport is executed as a worker (through thread-stream).
Currently, pino transports such as pino-elasticsearch or pino-socket emit events on the stream but when they are executed as a worker it's not possible to listen to these events (see #97).

With the following code, you can listen to the socketError if you are using thread-stream or not:

// transport
outputStream.emit('socketError', error)
if (parentPort) {
  parentPort.postMessage({ code: 'EVENT', name: 'socketError', args: [error] })
}

// application
const socketStream = pino.transport({
  target: "pino-socket",
  options: {
    address: tcpAddress,
    port: tcpPort,
    mode: "tcp",
    reconnect: true,
  }
})
// works even if socketStream is a ThreadStream
socketStream.on('socketError', (error) => {
  // do something!
})

Ideally, it should be transparent but as far as I know it's not possible to listen to events from another thread?