workflow: Use smaller logger dependency with override support

[andrewwormald]

Motivation

The motivation for this change is to begin trimming down the dependencies that workflow has. It forces good questions and healthy decisions as to what the core package needs and what it does not need. Since go's core library supported structured logging which is provided already and normalised the need to use https://github.com/luno/jettison became unclear especially as it brings some indirect dependencies itself.

It (https://github.com/luno/jettison) provides useful things beyond logging such as better errors than the stdlib error when it comes to tracing errors back but this should be a choice for the user of Workflow not something you need to use if you use workflow.

Research /Impact:

By creating a new project / go module, solely importing workflow (using the most current version [6c589eb]), implementing a simple: Start -> Middle ->End workflow, and then compiling it the output size is 8.2 Megabytes. When using the same project but using the changes via a go mod's replace command, the new output is 8.0Megabytes which is a 200 Kilobyte reduction. I think a 200Kilobytes shaved off an application is worth it if the user is content with the stdlib's logger

Removed dependencies

[echarrod]

LGTM, would be good to get @adamhicks's eyes on it too 👍

[adamhicks]

If you need to Wrap and New with metadata, you should be using jettison. Your implementation (in internal/errors/errors.go) of errors is notably different to jettison and has made different choices that ignore some of the nuances that jettison handles with wrapping and unwrapping errors.

If you don't really need to wrap metadata, then you don't need an error package and you should be able to expose a simpler Logger func or interface that allows the user to bring their own logger.

[andrewwormald]

If you need to Wrap and New with metadata, you should be using jettison. Your implementation (in internal/errors/errors.go) of errors is notably different to jettison and has made different choices that ignore some of the nuances that jettison handles with wrapping and unwrapping errors.

If you don't really need to wrap metadata, then you don't need an error package and you should be able to expose a simpler Logger func or interface that allows the user to bring their own logger.

The main concern around jettison is that it's errors are tightly coupled to the logger it provides. If jettison exported it's errors independently from it's logger then I would use that but I would ultimately like to make this leaner and allow the user to add bloat where it makes sense like providing a logger.

I will see if wrapping is really necessary as it would be first prize to not have a custom error type internally. I would like to use the std package.

[adamhicks]

If you need to Wrap and New with metadata, you should be using jettison. Your implementation (in internal/errors/errors.go) of errors is notably different to jettison and has made different choices that ignore some of the nuances that jettison handles with wrapping and unwrapping errors.
If you don't really need to wrap metadata, then you don't need an error package and you should be able to expose a simpler Logger func or interface that allows the user to bring their own logger.

The main concern around jettison is that it's errors are tightly coupled to the logger it provides. If jettison exported it's errors independently from it's logger then I would use that but I would ultimately like to make this leaner and allow the user to add bloat where it makes sense like providing a logger.

I will see if wrapping is really necessary as it would be first prize to not have a custom error type internally. I would like to use the std package.

What do you need from the error? Jettison has a couple of utilities to extract info.
We can look at making the logger / error coupling looser quite easily. We don't need to expose the error model to achieve this.

[andrewwormald]

If you need to Wrap and New with metadata, you should be using jettison. Your implementation (in internal/errors/errors.go) of errors is notably different to jettison and has made different choices that ignore some of the nuances that jettison handles with wrapping and unwrapping errors.
If you don't really need to wrap metadata, then you don't need an error package and you should be able to expose a simpler Logger func or interface that allows the user to bring their own logger.

The main concern around jettison is that it's errors are tightly coupled to the logger it provides. If jettison exported it's errors independently from it's logger then I would use that but I would ultimately like to make this leaner and allow the user to add bloat where it makes sense like providing a logger.
I will see if wrapping is really necessary as it would be first prize to not have a custom error type internally. I would like to use the std package.

What do you need from the error? Jettison has a couple of utilities to extract info. We can look at making the logger / error coupling looser quite easily. We don't need to expose the error model to achieve this.

From the error side I would like to wrap and add context around the error that can be propagated and as it is propagated more context can be added then logged at a top level. How the log is written and structured is a different concern. It would also be nice if the jettison module can be broken down into specific modules but its largely built as one thing. So for example I would like to avoid logger format dependencies that jettison uses if I just want to use jettison errors for example. You kind of need to commit to the whole package.

[adamhicks]

If you need to Wrap and New with metadata, you should be using jettison. Your implementation (in internal/errors/errors.go) of errors is notably different to jettison and has made different choices that ignore some of the nuances that jettison handles with wrapping and unwrapping errors.
If you don't really need to wrap metadata, then you don't need an error package and you should be able to expose a simpler Logger func or interface that allows the user to bring their own logger.

The main concern around jettison is that it's errors are tightly coupled to the logger it provides. If jettison exported it's errors independently from it's logger then I would use that but I would ultimately like to make this leaner and allow the user to add bloat where it makes sense like providing a logger.
I will see if wrapping is really necessary as it would be first prize to not have a custom error type internally. I would like to use the std package.

What do you need from the error? Jettison has a couple of utilities to extract info. We can look at making the logger / error coupling looser quite easily. We don't need to expose the error model to achieve this.

From the error side I would like to wrap and add context around the error that can be propagated and as it is propagated more context can be added then logged at a top level. How the log is written and structured is a different concern. It would also be nice if the jettison module can be broken down into specific modules but its largely built as one thing. So for example I would like to avoid logger format dependencies that jettison uses if I just want to use jettison errors for example. You kind of need to commit to the whole package.

I don't think that's true at all, I spent a decent amount of time separating out the errors and logging.
You can use jettison errors and errors.GetKeyValues to do what you want.

[andrewwormald]

If you need to Wrap and New with metadata, you should be using jettison. Your implementation (in internal/errors/errors.go) of errors is notably different to jettison and has made different choices that ignore some of the nuances that jettison handles with wrapping and unwrapping errors.
If you don't really need to wrap metadata, then you don't need an error package and you should be able to expose a simpler Logger func or interface that allows the user to bring their own logger.

The main concern around jettison is that it's errors are tightly coupled to the logger it provides. If jettison exported it's errors independently from it's logger then I would use that but I would ultimately like to make this leaner and allow the user to add bloat where it makes sense like providing a logger.
I will see if wrapping is really necessary as it would be first prize to not have a custom error type internally. I would like to use the std package.

What do you need from the error? Jettison has a couple of utilities to extract info. We can look at making the logger / error coupling looser quite easily. We don't need to expose the error model to achieve this.

From the error side I would like to wrap and add context around the error that can be propagated and as it is propagated more context can be added then logged at a top level. How the log is written and structured is a different concern. It would also be nice if the jettison module can be broken down into specific modules but its largely built as one thing. So for example I would like to avoid logger format dependencies that jettison uses if I just want to use jettison errors for example. You kind of need to commit to the whole package.

I don't think that's true at all, I spent a decent amount of time separating out the errors and logging. You can use jettison errors and errors.GetKeyValues to do what you want.

I think you might be confusing package with module. I am specifically referring to not needing to import the entire jettison module to access the errors package. (i.e https://github.com/luno/jettison/blob/main/go.mod is the single mod for all jettison's pakcages)

[adamhicks]

I think you might be confusing package with module. I am specifically referring to not needing to import the entire jettison module to access the errors package. (i.e https://github.com/luno/jettison/blob/main/go.mod is the single mod for all jettison's pakcages)

Jettison isn't large, I'm not sure why we'd need to do that. Do you have anything that I can look at to see what the benefit of doing that (adding more go.mod files) would be?

[andrewwormald]

Sorry havn't been able to get back to this but should be able to later this week

[andrewwormald]

@adamhicks I have stripped it back further to not have a custom error type (depending on how you look at it) for "workflow core" which is the go module github.com/luno/workflow. As shown below workflow core is the common denominator across user's implementations and i would like this to be as agnostic and vanilla Go as possible. There is no need to push for more than is needed, which is a design choice, for workflow's core. If you look at some of the adapters and their modules you will see that they still use jettison and do so because that is a choice to use that adapter. You can easily not use that adapter and create your own and that is why it's fine for it to exist there and not in the "core" module.

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
88.9% Coverage on New Code
0.6% Duplication on New Code

See analysis details on SonarCloud