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

Wrapping hclog around stdlib log #11

Closed
evan2645 opened this issue Sep 5, 2017 · 8 comments
Closed

Wrapping hclog around stdlib log #11

evan2645 opened this issue Sep 5, 2017 · 8 comments

Comments

@evan2645
Copy link
Contributor

evan2645 commented Sep 5, 2017

I am using hashicorp's go-plugin, which asks for hclog logger. I am not using hclog elsewhere, and wish to wrap the hclog interface around my stdlib-compliant logger, but it seems this is not possible?

I can set Output writer, however I cannot specify a mutex which is shared with my core logger as the hclog mutex is unexported. The only fix I can think of is to send a change which exposes a configurable mutex in LoggerOptions. Is this... kosher?

The whole thing is unfortunate really because it makes integrating with go-plugin very difficult if you're not using hclog, which is likely the case. It makes me wonder how others outside hashicorp are using these tools...? Anyways, please let me know if exposing an optional mutex is acceptable - I can send a patch if so. Thanks in advance

@calvn
Copy link
Member

calvn commented Sep 5, 2017

We do something similar in Vault (since it's still using logxi). You need to wrap your logger in a logger implementation that satisfies hclog's logger interface. You can then pass an instance of that to go-plugin's ClientConfig.Logger.

@evan2645
Copy link
Contributor Author

evan2645 commented Sep 5, 2017

Woof... I was afraid you might say something like that :)

This feels like a pretty big usability smell - much of the code in the wrapper is copy-pasted from hclog. I can build a similar shim, but it does seem like there should be a better solution provided by the hclog package. I can see benefit in this method over sharing a writer, however I think the benefit is diminished if the logger I'm using is unstructured.

Is there significant drawback to sharing a writer and mutex? It seems reasonable to provide a mutex since we can provide a writer. If you can think of any other solution that gives similar usability win, I'm happy to spend some cycles on it. Similarly, if I am better served to open an issue against go-plugin (since it ends up meaning folks wanting to use go-plugin probably have to write a 150-line shim), then I can do that too... happy to help any way I can

@jefferai
Copy link
Member

jefferai commented Sep 5, 2017

If you want a stdlib logger, why not use the StandardLogger function to get one?

@evan2645
Copy link
Contributor Author

evan2645 commented Sep 5, 2017

@jefferai That function works well when the project as a whole uses hclog and you need to generate a stdlib-compliant logger for a subsystem, but not the other way around (I have a stdlib-compliant logger in my project, and want to pass an hclog-compliant logger down to go-plugin). Perhaps I am missing something...? I'm hesitant to switch my core logger to hclog simply so that I can use go-plugin

@jefferai
Copy link
Member

jefferai commented Sep 5, 2017

I think originally we were also going to have a function for deriving an hclog logger from stdlib logger but didn't need it yet so it's not yet implemented. But also it depends entirely on having access to the underlying writer and that writer being able to be locked. That's pretty easy though, e.g. logxi uses https://godoc.org/github.com/mgutz/logxi/v1#ConcurrentWriter

@evan2645
Copy link
Contributor Author

evan2645 commented Sep 6, 2017

Got it... yea, I had thought of implementing something similar, but stdlib log doesn't expose mutex, and other/different libs expose it in different ways, so allowing the consumer to pass one in seemed to me like the best way to go while still remaining generic.

If this doesn't sound too objectionable, I can take a shot at sending a PR to expose this. I think it can easily be made backwards compatible.

@evan2645
Copy link
Contributor Author

evan2645 commented Sep 6, 2017

The change I had in mind was small enough that I just went ahead and did it (#12). Please feel free to close it if it's not the right approach

evanphx added a commit that referenced this issue Oct 5, 2017
[#11] Expose a configurable mutex for shared writers
@evan2645
Copy link
Contributor Author

Fixed with #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants