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

[Core] Elasticsearch compatible timezone defaults #89689

Closed
mshustov opened this issue Jan 29, 2021 · 7 comments
Closed

[Core] Elasticsearch compatible timezone defaults #89689

mshustov opened this issue Jan 29, 2021 · 7 comments
Assignees
Labels
Feature:Logging Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Jan 29, 2021

Kibana legacy logging supports timezone configuration with default to UTC timezone:

logging.timezone | Set to the canonical time zone ID (for example, America/Los_Angeles) to log events using that time zone.
For possible values, refer to database time zones.
Default: UTC

However, I talked to @pgomulka about default ES settings and it seems to be different:

from what I see in code and when running the Timezone should be local to the host.
However, if you run from docker it looks it is a UTC timezone.

To simplify debugging, Kibana should align settings with Elasticsearch.

We need to update the date formatting

moment().toISOString(keepOffset);

and document how customers can change timezone via config

pattern: "%d{ISO8601}{GMT}..."
@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Logging labels Jan 29, 2021
@elasticmachine
Copy link
Contributor

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

@Bamieh
Copy link
Member

Bamieh commented Feb 2, 2021

Can we just change the default to system timezone instead of utc?

@TinaHeiligers
Copy link
Contributor

@restrry This issue talks about changing the default timezone behavior of the legacy logging system but going back to previous offline discussions, the question that was raised was about the KP logging system.
Do we want to change both the legacy and the KP default date TZ to use local rather than UCT or just legacy or just KP?

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Feb 2, 2021

@lukeelmers @restrry I have a few questions that I'd like to bounce off of you:

Pre-amble:
For keeping parity with the way ES stores dates, the default time format in KP logging is in UTC. Meaning that wherever in the world the kibana server is running, all times are still reported (and stored) in UCT.

This is not anticipated to change (or is it?)

Current behavior
When no format is set, we default to: dateFormat: string = formats.ISO8601, which returns: momentDate.toISOString().
Example output:2021-01-06T23:34:57.034Z

Observations
If we change the behavior to retain the offset (as described in this issue) and keep the default as: dateFormat: string = formats.ISO8601, what we’ll be doing is returning momentDate.toISOString(true), i.e., keepOffset is true in momentDate.toISOString(keepOffset).
Example output: 2021-01-06T16:34:57.034-07:00 (for a server in Phoenix)

If I'm interpreting our formatting correctly, this is the same format as what would be returned if we changed the default to be dateFormat: string = formats.ISO8601_TZ. The return from using ISO8601_TZ is: momentDate.format('YYYY-MM-DDTHH:mm:ss.SSSZ’)
Example output: 2021-01-06T16:34:57.034-07:00 (for a server in Phoenix)

If we hard coded the returned default format to be in the current timezone with the offset, i.e. changed
https://github.com/elastic/kibana/blob/master/src/core/server/logging/layouts/conversions/date.ts#L32
momentDate.toISOString() -> momentDate.toISOString(true), we'll end up making %date{ISO8601} and %date{ISO8601_TZ} redundant. Users won't have a way to override the format to remove the timezone.

e.g.
With the current defaults and using the following patterns for logging the metrics.ops context, we get:

pattern date format timestamp
%date{ISO8601} [2021-02-03T00:02:01.920Z]
%date{ISO8601_TZ} [2021-02-02T17:02:01.920-07:00]
%date{ISO8601}{GMT} [2021-02-03T00:02:01.920Z]

Changing

case formats.ISO8601:
      return momentDate.toISOString();

to

case formats.ISO8601:
      return momentDate.toISOString(true);

the same pattern-format output would be:

pattern date format timestamp
%date{ISO8601} [2021-02-02T17:09:09.917-07:00]
%date{ISO8601_TZ} [2021-02-02T17:02:01.920-07:00]
%date{ISO8601}{GMT} [2021-02-03T00:09:09.917+00:00]

Is it not then better to rather change the default format in the date conversion to fallback to ISO18601_TZ rather than hard code retaining the offset in the ISOString?

Please let me know if I'm completely off the mark here. Timezones and I don't have the greatest track record 😊

@lukeelmers
Copy link
Member

Is it not then better to rather change the default format in the date conversion to fallback to ISO18601_TZ rather than hard code retaining the offset in the ISOString?

@TinaHeiligers That's a good point - providing the offset argument to toISOString(true) is going to always format that string as if you were to have used ISO8601_TZ anyway. I don't think we'd want to make ISO8601 and ISO8601_TZ redundant.

If the goal is to have logs default to the host timezone like ES does, it sounds like the main change that needs to happen would be to do moment.guess() if the timezone isn't explicitly provided and the ISO8601_TZ format is being used.

Then we could either:

  1. Make ISO8601_TZ the default format as you suggest (and users could opt-out by manually setting %date{ISO8601} in their pattern, or by applying a different timezone in their pattern)
  2. Or I guess you could alternatively change the format for ISO8601 to momentDate.tz(timezone).format('YYYY-MM-DDTHH:mm:ss.SSS'), which would apply the timezone to an ISO string without the offset.

IMO option (2) sounds like potential for a debugging nightmare -- If you are looking at a log with a timestamp that's anything but GMT, you probably are going to want to see that offset -- so I'd probably go with (1).

I suppose we would also want to deprecate logging.timezone at the same time. I'm a little confused on the history here, as our breaking changes docs say that in 8.0 We now use the host machine’s timezone by default, but AFAICT from the PR introducing that change we actually set the default to UTC, which still appears to be the case in the legacy logging system. So no matter what we do here, we probably should change that note in the docs 😄

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Feb 3, 2021

Kibana legacy logging supports timezone configuration with default to UTC timezone

@restrry The legacy logging timezone default was changed from UTC to the host timezone in #22696. It appears that the documentation wasn't updated to reflect the change. As it stands right now, logging.timezone is optional (internally falling back to the local timezone of the host) and we need to update the docs to reflect that.

I suppose we would also want to deprecate logging.timezone at the same time.

@lukeelmers I agree that the history of logging.timezone is confusing 😕 and, unless I'm missing something, we don't take it into account when logging from the KP. Instead, we leave it up to the user to define the timezone directly in the %date modifier in the date pattern: [@date{ISO8601_TZ}{America/Phoenix}].

Without any docs on new logging configuration, giving details on how to set the timezone might not be very useful (unless folks actually dig through to the logging README) and I think we need to wait until we add these before deprecating logging.timezone.

Would we want to remove logging.timezone from core config at the same time as deprecating it? If we do, I think we should have a custom deprecation (e.g. similar to requestLoggingEventDeprecation) that gives details about how to set the timezone in the log date modifier.

WDYT?

@TinaHeiligers
Copy link
Contributor

Closed by #90368

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

5 participants