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

[Logging] Legacy and new system configuration #129381

Closed
Tracked by #134169
afharo opened this issue Apr 4, 2022 · 19 comments
Closed
Tracked by #134169

[Logging] Legacy and new system configuration #129381

afharo opened this issue Apr 4, 2022 · 19 comments
Assignees
Labels
docs Feature:Logging Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@afharo
Copy link
Member

afharo commented Apr 4, 2022

In our user-facing docs about the logging feature (for 7.17, since it was removed for 8.0): https://www.elastic.co/guide/en/kibana/7.17/logging-settings.html

It looks like we don't make a big emphasis on the key differences, and neither warns of the potential unexpected issues that may occur if we have settings for both logging systems.

An example is a use case we recently knew about a user had set the new system with log-rotation, but the "old" log was still actively being written because they had logging.dest set up.

  • Should we solve it with a big callout in the docs?
  • Should we crash Kibana if we identify configuration set for both systems at the same time?

We already log a deprecation warning for logging.dest, but it doesn't warn that having it enabled might affect other features like this case.

@afharo afharo added Team:Docs Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Logging docs labels Apr 4, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-docs (Team:Docs)

@afharo afharo added the discuss label Apr 4, 2022
@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Apr 4, 2022

At the time we enabled the new logging config we had to retain support for the legacy logging system while deprecating it. The new logging system has an entirely different approach and I don't think we knew what issues could be caused when both were enabled, especially when it comes to using the file-type appender.

We could add a warning to the docs around potential issues (the ones we have already seen) but I'm sure there are others that the docs won't specifically cover.

Maybe we should mention that the switch to the new logging system is assumed to be an 'all-or-nothing' approach?
FWIW, the overview of Kibana configuration settings in 7.17 only contains the new logging system configuration (a subset) and the deprecations were added to the 7.13 release notes. Should we carry over those breaking changes as warnings in the docs for version 7.14+?

@afharo
Copy link
Member Author

afharo commented Apr 4, 2022

IMO, this is a special case of deprecated config parameters because, as opposed to other params that are either not used or renamed, they actually affect 2 coexisting services that may interfere to each other.

That's why I think we need special handling of this situation: either via docs, making it very clear that it should be "all-or-nothing". Or we could even go one step further and crash Kibana if we notice mixed usages.

What do you think?

@pgayvallet
Copy link
Contributor

pgayvallet commented Apr 5, 2022

Should we crash Kibana if we identify configuration set for both systems at the same time

Definitely not ihmo. The legacy and new logging system were meant to be able to coexist during the 7.x 'transition' period (via the 'legacy' appender). AFAIK, it may even be quite complex to try to identify if configurations are provided for both, given most of the config conversion is done in the lower-level legacy config adapter (please don't ask), before core even accesses it. Also, some of our tests on 7.x were effectively using both systems at the same time (and some users might)

If we did remove the legacy logging configuration from our public documentation on the 7.last branch though, this was a terrible mistake.

@lukeelmers
Copy link
Member

Agreed Kibana shouldn't crash if both configs are provided -- technically that should still be supported to ease the transition, even it isn't recommended to mix old & new configs. And yeah, we should definitely have legacy logging config info in all 7.x docs as they are still valid.

IMO what would be useful here is writing a guide-like doc around "how to prepare for kibana's new logging system in 8.0". In that, we could emphasize that we recommend completely switching over from old to new configs at the same time, rather than having a mix of the two. And we could call out log rotation as a "gotcha" that folks need to be careful about if they are trying to use both old & new systems simultaneously.

@PengYi-Elastic
Copy link

I saw a config as follows on 7.16 without logging.dest setup but the "old" log was still actively being written. I suspect there should something else to trigger legacy logging.

server.host: "0.0.0.0"
elasticsearch:
  hosts:
    - "https://xxxxx1:9200"
    - "https://xxxxx2:9200"
    - "https://xxxxx3:9200"
username: "kibana_system"
server.ssl:
  enabled: true
  certificate: /etc/kibana/public.crt
  key: /etc/kibana/private.key
server.publicBaseUrl: "https://xxxxx:5601"
elasticsearch.ssl:
  certificate: /etc/kibana/public.crt
  key: /etc/kibana/private.key
  certificateAuthorities:
    - /etc/kibana/ca.crt
i18n.locale: "ja-JP"
logging:
  appenders:
    rolling-file:
      type: rolling-file
      fileName: /var/log/kibana/kibana.log
      policy:
        type: time-interval
        interval: 300s 
        modulate: true
      strategy:
        type: numeric
        pattern: '-%i'
        max: 10
      layout:
        type: pattern
  root:
    appenders: [default, rolling-file]

@afharo
Copy link
Member Author

afharo commented May 17, 2022

@PengYi-Elastic, options can also be provided via CLI --loging.dest=true. In those cases, I'd review the service definition / running script. We've seen that in the past for old installations.

@PengYi-Elastic
Copy link

@afharo , thanks for the direction! It really helps :)

@TomonoriSoejima
Copy link
Contributor

TomonoriSoejima commented May 24, 2022

I just confirmed in 7.16.0, indeed there is --logging.dest="/var/log/kibana/kibana.log" in the service script.

[Unit]
Description=Kibana
Documentation=https://www.elastic.co
Wants=network-online.target
After=network-online.target

[Service]
Type=simple
User=kibana
Group=kibana

Environment=KBN_HOME=/usr/share/kibana
Environment=KBN_PATH_CONF=/etc/kibana

EnvironmentFile=-/etc/default/kibana
EnvironmentFile=-/etc/sysconfig/kibana

ExecStart=/usr/share/kibana/bin/kibana --logging.dest="/var/log/kibana/kibana.log" --pid.file="/run/kibana/kibana.pid" --deprecation.skip_deprecated_settings[0]="logging.dest"

Restart=on-failure
RestartSec=3

StartLimitBurst=3
StartLimitInterval=60

WorkingDirectory=/usr/share/kibana

StandardOutput=journal
StandardError=inherit

[Install]
WantedBy=multi-user.target

@afharo
Copy link
Member Author

afharo commented May 24, 2022

👆 cc @elastic/kibana-operations

@spalger
Copy link
Contributor

spalger commented May 26, 2022

@jbudz Sounds like we might need to fix the startup args passed in the 7.x service scripts

@afharo please provide at least some relevant info about what you're pinging people about when adding them to a long issue like this 😅 What change do we need to make here?

@jbudz
Copy link
Member

jbudz commented May 26, 2022

We've removed the startup arguments in kibana.service in 8.0 as a breaking change. Moving the logging configuration to kibana.yml in 7.x is problematic because there's a decent chance on upgrade users will opt to preserve their old kibana.yml, and logging will be shut off.

Sorry about this all, it's an unfortunate longer lasting bug. Open to better UX here but we have to be careful not to break things.

Removing the CLI argument from /etc/systemd/system/kibana.service followed by systemctl daemon-reload is an in-place workaround.

@afharo
Copy link
Member Author

afharo commented May 30, 2022

@afharo please provide at least some relevant info about what you're pinging people about when adding them to a long issue like this 😅 What change do we need to make here?

@spalger apologies! Taking a mental note for the next one :)

We've removed the startup arguments in kibana.service in 8.0 as a breaking change. Moving the logging configuration to kibana.yml in 7.x is problematic because there's a decent chance on upgrade users will opt to preserve their old kibana.yml, and logging will be shut off.

@jbudz I completely agree! Probably removing it from 7.x's service can be considered a breaking change and we shouldn't remove it. However, I'm wondering if we should stop silencing the deprecation warning: should we remove --deprecation.skip_deprecated_settings[0]="logging.dest"?

Removing the CLI argument from /etc/systemd/system/kibana.service followed by systemctl daemon-reload is an in-place workaround.

👆 @elastic/kibana-core we might need to add this to the documentation in the 7.x branches.


Summarizing the scope of this issue per discussion so far:

We need to update the documentation for 7.x to make it clear that, while we support both logging systems being enabled at the same time, there are side effects the users should be aware of:

  1. Duplication of entries: if both logging systems are writing to the same appenders (i.e.: console or file://kibana.log) they may see duplicated entries in the logs.
  2. Log rotation: when both logging systems are enabled and log-rotation is enabled, it may occur that one system applies the rotation while the other still writes to the original file.
  3. CLI options vs. kibana.yml: we need to make users aware that logging.dest might be set via CLI options in the kibana.service declaration. If they want to stop using the legacy logging system, they should remove the CLI argument from /etc/systemd/system/kibana.service and run systemctl daemon-reload.

TBD: Should we remove --deprecation.skip_deprecated_settings[0]="logging.dest" to warn the users when they are using it?

@jbudz
Copy link
Member

jbudz commented May 31, 2022

Context on skip_deprecated_settings - #111263. There were concerns that the upgrade assistant would give unactionable warnings. The service scripts will manage this automatically on upgrading to 8.0, so there was a decision to hide the warning.

@afharo
Copy link
Member Author

afharo commented May 31, 2022

Thanks for the explanation @jbudz!

@elastic/kibana-core IMO, that covers the "happy path" (a user with no special logging configuration upgrades from 7.17 to 8.0). However, I wonder if a user running 7.17 and wants to test out the new logging system, logging.dest is set up for them and they don't have any info about it (we are proactively silencing the deprecation warning). Should we have a special deprecation warning that will still log if it notices both configuration versions are coexisting?

@lukeelmers
Copy link
Member

Should we have a special deprecation warning that will still log if it notices both configuration versions are coexisting?

Part of the reason for introducing skip_deprecated_settings was so that we don't warn users about using a deprecated setting that they didn't explicitly use on their own, so I'm not sure if unskipping logging.dest is the way to go here.

To me I think this discussion has a few root causes:

  1. We didn't have clear documentation about the breaking changes affecting kibana.service
  2. We accidentally removed legacy logging docs from 7.last
  3. We don't have a callout explicitly mentioning the particular issue of using the new system's log rotation with the legacy logging.dest still in place.
    • AFAIK this is the only real issue we are aware of with mixing the configs (correct me if I've forgotten something). Otherwise you should be able to use both old & new style together, or at least that's what we had intended.

Considering 1 has now been fixed, I'd propose we:

  • Add the legacy logging docs back to the 7.17 docs
  • Add a section to the beginning of logging configuration migration with an overview of how to prepare for the 8.0 system. In that section, we should specifically mention that:
    • It is possible to mix the usage of old & new config styles, however we recommend changing them all at once
    • If you are using log rotation, you need to be careful when mixing old & new config styles, as it could lead to unexpected behavior. You also need to be careful if you are using the service scripts (kibana.service)

@lukeelmers
Copy link
Member

We should also make sure #134462 is clarified as a part of our docs updates

@rudolf
Copy link
Contributor

rudolf commented Jun 15, 2022

We should also change the location of https://www.elastic.co/guide/en/kibana/7.17/logging-configuration-migration.html so that it's not located under Developer Guide -> Architecture since this is applicable to all users not just developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Feature:Logging Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

10 participants