search

hashicorp / go-plugin

Golang plugin system over RPC.
Mozilla Public License 2.0
5.25k stars 450 forks source link

Change log level to trace when stdio not implemented #142

Closed marefr closed 4 years ago

marefr commented 4 years ago

Doesn't make sense to log a warning when stdio service is not implemented, that is when plugin compiled with older go-plugin version.

I'm not quite sure what the stdio service actually provides for a plugin compiled with latest version? Does it change logs to be shipped over gRPC instead of stderr?Fix

Ref #135 Fixes https://github.com/grafana/grafana/issues/23461

mitchellh commented 4 years ago

I'm not sure trace level is correct here. I think a warning is still correct as a way to highlight that stdio syncing isn't available, since its a one-time log message. Info could work and is maybe correct here since I could see a warning implying something is wrong (I don't view it that way but could understand that viewpoint).

The stdio service transfers stdio streams (stdout/stderr) from the logs to an alternate io.Writer. This is explained in #135.

marefr commented 4 years ago

Thanks. Will change to info.

I still think usage of syncing of stderr/stdout over gRPC is something only a few plugins will use and should be an opt-in feature. For example in the case of writing plugins without Go you need to implement the stdio service now to not get the warning message. In some languages can be tricky to implement. It's also not mentioned in https://github.com/hashicorp/go-plugin/blob/master/docs/guide-plugin-write-non-go.md as a requirement, but not implementing it result in this warning/info message.

mitchellh commented 4 years ago

I agree. It is an opt-in feature already. The client sets SyncStdout (or err) if they want to opt-in (it defaults to /dev/null equivalent). The plugin can choose to enable or disable the service, its backwards compatible.

I don’t think 3 extra log lines noting the service isn’t available is breaking any compatibility, and we purposely put them in there originally just so you know that it isn’t available (even if you didn’t want it).

I do agree we can re-level them a bit.

Net net though: no one should have to implement this service, and I don’t think anything says they have to today.

marefr commented 4 years ago

We muted sdtio in https://github.com/grafana/grafana/issues/23461 so closing this.