Support for structured logging

[mwasilew2]

• adds support for structured (JSON) logs
• adds support for levelled logging
• switches to using slog

Note: there's 74 candidates for switching from fmt.Println to log.*

> grep -riI 'fmt\.' . | grep -v "return" | grep -v "fmt.Spr" | grep -v "fmt.Erro" | wc -l
74

If we decide to do it, I think it should be in a separate PR.

[mwasilew2]

blocked by: #184

[tomasmik]

I had a quick read through the documentation of slog and I'm not sure if we actually need it? The json logging is awesome, I guess, but again is that ever useful here? When you get an error its usually just a single string and there isn't really that much useful context to provide imo.

But anyway that's not my main concern. My actual main concern is that this is potentially breaking the output contract and is adding inconsistency because we'll keep using fmt and slog now. What I mean is that, there are people who've written shell scripts to interact with spacectl, I'm afraid if we change the output, we might affect those. Maybe I missed something in the docs of slog though?

[tomasmik]

Nit: It's kinda nicer when flags are defined vars as we can later use those to extract values from context: https://github.com/spacelift-io/spacectl/blob/main/internal/cmd/module/create_version.go#L13

You could just define them above main here 🤷🏻

[mwasilew2]

@tomasmik

The json logging is awesome, I guess, but again is that ever useful here?

IMHO it's crucial when doing log analysis and it's super useful when automating things, e.g.

there are people who've written shell scripts to interact with spacectl,

I bet it's not just shell scripts, but also CI/CD pipelines. Parsing those logs if they are not structured can be really painful. Even if using text format and not JSON, the columns are easier to work with when using slog rather than fmt.Println(). We could instrument the cli with usage metrics to get some data on how it's actually being used, but that brings in a huge set of problems of its own.

My actual main concern is that this is potentially breaking the output contract

You're absolutely right and the way I think about this is: do we want to stick with fmt.Println() for the rest of the life of this tool? Maybe not. If not then how do we introduce this change? Well, one approach is to say: we're on 0.x version and it's never been advertised as being stable, so yeah, it will break output for some people. Another way is to say: it's not been officially called stable, but we know that a lot of people are relying on it, so let's start preparing a 1.x release and this change would be one of the breaking changes included. Another approach is to put structured logging behind a feature flag for now, but that's non-trivial. At the moment, outputting is not using any interface so we can't easily change how we print things. Yet another way is to say that we'll be introducing a set of libraries that will provide easy-to-use implementations of functionalities shared across our projects and slog is one of them.

Obviously, there are more ways to approach it and to me, it looks like a judgment call on how fast you want to move vs. how much you want to avoid breaking things. Should we get more feedback from the team?

adding inconsistency because we'll keep using fmt and slog now

That's what I was concerned about as well, but it's a significant effort to switch away from fmt and I wanted to start the conversation before putting in the effort.

[tomasmik]

@mwasilew2, great response with valid points there.

do we want to stick with fmt.Println() for the rest of the life of this tool? Maybe not. If not then how do we introduce this change?

I do not own spacectl so I'd suggest to bring this up in a discussion ticket so we can talk through it. I'm not opposing this idea and I can see some benefit to this, but then I'm also not sure about it and as it requires a decent amount of effort, we should discuss it internally.

[adamconnelly]

I think overall this change is fine. I don't think there's an issue with altering the contract here because the logging is going to stderr rather than stdout. The main thing that could break the contract would be if, for example, we ended up sending log messages to stdout for a command like spacectl stack list --output json.

So basically in my mind there's two things at play here:

1. Log output (progress indications, errors, etc).
2. Command output (e.g. the stack list command I referenced previously).

I think it's entirely reasonable for someone to rely on command output as part of automation, and we need to be careful when changing that, but I don't think there's any contract in place when it comes to log output.

Ok, maybe three things actually - we also need to care about what the usability is like when not using spacectl in automation. For example, if I'm a human just using spacectl to query something quickly, I definitely don't want JSON output, and I probably want the output to be pretty minimal so it's not overwhelming.