[cmd/opampsupervisor] log stderr using Error

[codeboten]

Description

This seems like a bug, but it is also a breaking change. The logger used when logging errors from the collector was always logging messages with a severity of Info, even for messages from the stderr pipe.

[TylerHelmuth]

I agree with this classification, although it will result in anyone using passthrough logs but not configuring the collector to use stdout and stderr appropriately to start seeing a bunch of error logs.

[evan-bradley]

not configuring the collector to use stdout and stderr appropriately

My understanding is that the Collector outputs to stdout for anything related to the CLI (e.g. the components command) and to stderr for application-level logs. How should users configure this?

[dpaasman00]

I don't totally understand this point either, but what comes to mind is if the user is configuring the collector's own telemetry output path to be just "stderr" like so:

service:
  telemetry:
    logs:
      level: DEBUG
      output_paths: ["stderr"]

Then DEBUG level and above collector logs would appear as errors in the supervisor's logs. Is this the kind of bad configuration we're talking about?

[TylerHelmuth]

I believe by default the collector uses stderr when running for all logs: https://github.com/open-telemetry/opentelemetry-collector/blob/b1ce36b19f7f95fd2689af2038820cc12066310f/service/telemetry/factory.go#L95-L96

This can be changed like @dpaasman00 mentioned. So a user would do

service:
  telemetry:
    logs:
      output_paths: ["stdout"]
      error_output_paths: ["stderr"]

[evan-bradley]

Thanks for the info.

@codeboten would it be reasonable to add that config to the default configuration the Supervisor passes to the Collector as part of this change? I have the same concern as Tyler about the Supervisor printing error-level logs using non-error level logs from the Collector.

[codeboten]

@evan-bradley sure I can add the output paths to correctly send to stderr and stdout if left unset

[codeboten]

set defaults for output_paths to stdout and for error_output_paths to stderr, ptal

[evan-bradley]

If I understand correctly, all Collector logs are printed to stderr, which would make Info the right log level, but it's possible I'm missing something.

@dpaasman00 would appreciate your input here since you implemented this functionality.

[dpaasman00]

I think the current behavior is a bug and this is a good change (sorry about that). To your point Evan, as I understand it, not all logs get printed to stderr. Similar to the example config I gave in the other comment thread, the user could use "stdout" or some log file and not log to stderr. Unless there's some other aspect being considered that I'm unaware of.

I don't really follow why this is a breaking change. Is it because it could break any processing being done with these logs today that has the assumption that they're always INFO logs but now they can also be ERROR?

[codeboten]

I don't really follow why this is a breaking change. Is it because it could break any processing being done with these logs today that has the assumption that they're always INFO logs but now they can also be ERROR?

Right, if someone was expecting INFO logs and not ERROR logs, this could potentially break their setup. Like i said though, i think this is more a bugfix than anything else.

[codeboten]

Tested this change w/ the latest config defaults of setting output paths and error output paths. The result is a bit mixed. The configuration of error_output_paths doesn't behave the way I expected it. As per the zap docs:

// ErrorOutputPaths is a list of URLs to write internal logger errors to.
// The default is standard error.
//
// Note that this setting only affects internal errors; for sample code that
// sends error-level logs to a different location from info- and debug-level
// logs, see the package-level AdvancedConfiguration example.

This means that w/ this change, errors from the fallback logger end up being reported as error and any logs after that will be reported as info unless someone configured the output_paths to stderr.

All in all, i think this change is still a net good to have, but there will be additional fixes needed to get logs reported at the correct level. I'll be opening an issue in the core repo, and will likely follow up this PR w/ a temporary solution for parsing the log level.

[evan-bradley]

Thanks for the additional info @codeboten.

Overall I'm a bit unsure about blanket printing all Collector logs as errors by default after this change before we tune things further, but I see the logic in making this change on its own. It sounds like we have consensus this is a net positive, so I think we're good to move forward.

I also agree this is more of a bug fix than a breaking change, but I don't mind calling more attention to it since it could cause a large increase in error logs for some users.