manticoresoftware / manticoresearch-helm

Helm chart for Manticore Search
Apache License 2.0
34 stars 10 forks source link

Pipe searchd.log to stderr by default? #22

Open barryhunter opened 2 years ago

barryhunter commented 2 years ago

I know we can configure this, be redefining values.config.content, but wonder if there is virtu in piping searchd to stderr by default? (leaving query log to stdout)

log = /dev/stderr

https://github.com/manticoresoftware/manticoresearch-helm/blob/master/chart/values.yaml#L66

... This makes it much easier to inspect the logs, in systems that can separate stderr/stdout streams. We use loki to ingest logs from containers.

sanikolaev commented 2 years ago

@ksa-real what's your take on this?

ksa-real commented 2 years ago

I don't have a strong opinion. When exporting to Loki or Elastic, the stream name is just another label. If using structured logging, the same effect can be achieved using an explicit label identifying the message type, e.g. type="query", or query="select some from ...". We are particularly looking into OpenTelemetry and structured logging. I'd say if using unstructured logging, do whatever is common in the industry.

sanikolaev commented 2 years ago

I'm not sure it's right to output non-errors to STDERR. Is there any official docker image or a helm chart which does that by default?

I see here https://docs.docker.com/config/containers/logging/ that:

The official nginx image creates a symbolic link from /var/log/nginx/access.log to /dev/stdout, and creates another symbolic link from /var/log/nginx/error.log to /dev/stderr, overwriting the log files and causing logs to be sent to the relevant special device instead. See the Dockerfile.

The official httpd driver changes the httpd application’s configuration to write its normal output directly to /proc/self/fd/1 (which is STDOUT) and its errors to /proc/self/fd/2 (which is STDERR). See the Dockerfile.

but nginx and httpd send errors to STDERR, not regular messages.

barryhunter commented 2 years ago

It is from nginx that learnt the practice :) But it is true that searchd.log is not all 'errors'.

My goal at least is be able to seperate the logs. With both searchd and the query log intermingled its hard to seperate them.

In theory query logs are easy to seperate as they have the comment at the start, but we have multiline queries, so there are lines that are hard to identify. Myabe as an alternateve then could have the query log 'whitespace normalized' to remove newlines ?

sanikolaev commented 2 years ago

My goal at least is be able to seperate the logs

As you said, you can configure this. I've discussed this with the team and we wouldn't like to change the defaults to avoid confusing users by the fact that non-errors can be only see in STDERR by default.

I'm also not sure we need query_log = /dev/stdout in the default config in the helm chart. We recently disabled default query logging in the official docker, so it makes sense to disable it in the helm chart too.

barryhunter commented 2 years ago

Ok, was just hoping it might help others. Was surprised when had an issue, and needed to look at searchd.log, that it was all intermingled.

Would also be nice if hte logging could be configured more directly, not just having to redefine the entirety of the searchd {} section. If/when a new chart is released, will have to check if the default searchd config has changed, and update our copy

For background using Flux Helm Controller, so have an additional file, that applies an 'overlay' to values.yaml. https://gist.github.com/barryhunter/5a6c364591d2e874934d2c1c592c307d Rather than that just editing values.yaml directly. (if was just editing values.yaml directly could use git tools to merge changes when change to new release. ) ... so have to duplicate the searchd config into our config, and maintain it long term.