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

[Data Plugin] Allow server-side date formatters to accept custom timezone #70668

Merged
merged 17 commits into from
Jul 13, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jul 2, 2020

Summary

Related to: https://github.com/elastic/kibana/pull/29977/files and adds the timezone formatting capability for the date_server field formatter.

When Advanced Settings shows the date format timezone to be "Browser,"
this means nothing to field formatters in the server-side context. The
field formatters need a way to accept custom format parameters. This
allows a server-side module that creates a FieldFormatMap to set a
timezone as a custom parameter. When custom formatting parameters exist,
they get combined with the defaults.

Changes:

  1. Copy DateNanosFormat from being a "common" field formatter to being 2 different formatters for browser and server
  2. Add setCustomParams to the FieldFormatsRegistry class

For context, this change is needed by this PR: https://github.com/elastic/kibana/pull/67027/files#diff-fc7a35232cdea756fb74dd6433e1e086R94

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@@ -40,7 +39,6 @@ export const baseFormatters: FieldFormatInstanceType[] = [
BoolFormat,
BytesFormat,
ColorFormat,
DateNanosFormat,
Copy link
Member Author

Choose a reason for hiding this comment

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

DateNanosFormat should not be a "common" formatter, since it requires different server-side behavior. It needs this because of the possibility that dateFormat:tz setting could be Browser

* baked in.
* We need to set the timezone manually here. The date is taken in as
* UTC and converted into the desired timezone. */
let date;
Copy link
Member Author

Choose a reason for hiding this comment

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

This section is copied from the date_server file

@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 Team:AppArch labels Jul 2, 2020
…zone

When Advanced Settings shows the date format timezone to be "Browser,"
this means nothing to field formatters in the server-side context. The
field formatters need a way to accept custom format parameters. This
allows a server-side module that creates a FieldFormatMap to set a
timezone as a custom parameter. When custom formatting parameters exist,
they get combined with the defaults.
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Changes, as far as I can grok them, seem to be good

@tsullivan
Copy link
Member Author

@mattkime @lizozom can you let me know if the general approach I explored here seems reasonable from the App Arch side? If so, I will keep pursuing this PR to make it ready for merge.

If yes, then I will need to work on fixing the functional tests. I'm not sure but it looks like I have broken date_nanos formatting for other plugins. I could use a pointer on where to look there, or figure out who to ask.

@mattkime
Copy link
Contributor

mattkime commented Jul 8, 2020

@tsullivan looks good to me!

@tsullivan tsullivan marked this pull request as ready for review July 8, 2020 21:57
@tsullivan tsullivan requested a review from a team as a code owner July 8, 2020 21:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)


return this.getInstance(conf.id, conf.params);
return this.getInstance(conf.id, instanceParams);
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows timezone to be passed from the browser and formatted correctly on the server

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan
Copy link
Member Author

@elastic/kibana-app-arch @mattkime would anyone be able to give the final approval for this? I have a PR targeted for 7.9 that depends on this. Thanks!

@tsullivan tsullivan merged commit 3222951 into elastic:master Jul 13, 2020
@tsullivan tsullivan deleted the data/allow-custom-formatting branch July 13, 2020 21:53
tsullivan added a commit to tsullivan/kibana that referenced this pull request Jul 13, 2020
…zone (elastic#70668)

* [Data Plugin] Allow server-side date formatters to accept custom timezone

When Advanced Settings shows the date format timezone to be "Browser,"
this means nothing to field formatters in the server-side context. The
field formatters need a way to accept custom format parameters. This
allows a server-side module that creates a FieldFormatMap to set a
timezone as a custom parameter. When custom formatting parameters exist,
they get combined with the defaults.

* add more to tests - need help though

* simplify changes

* api doc changes

* fix src/plugins/data/public/field_formats/constants.ts

* rerun api changes

* re-use public code in server, add test

* fix path for tests

* weird api change needed but no real diff

* 3td time api doc chagens

* move shared code to common

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
tsullivan added a commit that referenced this pull request Jul 14, 2020
…zone (#70668) (#71556)

* [Data Plugin] Allow server-side date formatters to accept custom timezone

When Advanced Settings shows the date format timezone to be "Browser,"
this means nothing to field formatters in the server-side context. The
field formatters need a way to accept custom format parameters. This
allows a server-side module that creates a FieldFormatMap to set a
timezone as a custom parameter. When custom formatting parameters exist,
they get combined with the defaults.

* add more to tests - need help though

* simplify changes

* api doc changes

* fix src/plugins/data/public/field_formats/constants.ts

* rerun api changes

* re-use public code in server, add test

* fix path for tests

* weird api change needed but no real diff

* 3td time api doc chagens

* move shared code to common

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants