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

[server/logging] Allow opting out of UTC #14705

Merged
merged 5 commits into from
Nov 8, 2017

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Nov 1, 2017

Closes #8499

Rather than always use UTC the timestamps in the log, this adds the logging.useUTC config option, which defaults to true (until we get to #12175)

@spalger spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review v6.1.0 v7.0.0 labels Nov 1, 2017
@spalger spalger requested review from kimjoar and azasypkin November 1, 2017 19:09
@@ -46,10 +46,19 @@ export default class TransformObjStream extends Stream.Transform {
next();
}

formatTimestamp(data, format) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/question: is there any reason that prevents you from passing just @timestamp field instead of entire data object. Currently this method has potential access to the info that it shouldn't have + IMO with this method signature it looks more like extractAndFormatTimestamp :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like assuming that the "data" has a @timestamp property, which is why formatTimestamp does the property access, it is defined in the same place as the original @timestamp property is defined. Switched the method to extractAndFormatTimestamp()

]);

const { '@timestamp': timestamp } = JSON.parse(result);
expect(timestamp).to.eql(moment.utc(time).format());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional nit: we can use to.be here and below to be even more "stricter" :)

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just two nits.

@spalger spalger merged commit b79ba98 into elastic:master Nov 8, 2017
@spalger spalger deleted the implement/logging/out-out-of-utc branch November 8, 2017 15:44
spalger added a commit that referenced this pull request Nov 8, 2017
* [server/logging] make use of UTC configurable

* [server/logging/tests] try removing TZ from moment

* [server/log/formatJson] use strict equality checks

* [server/log/format] note in method name that time is extracted

(cherry picked from commit b79ba98)
@spalger
Copy link
Contributor Author

spalger commented Nov 8, 2017

6.x/6.1: abab9fc

@alexfrancoeur
Copy link

YES @spalger!!! 🍻 🥂 🎆

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

Successfully merging this pull request may close these issues.

4 participants