[Data Plugin] Allow server-side date formatters to accept custom timezone #70668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsullivan
commented
Jul 2, 2020
| @@ -40,7 +39,6 @@ export const baseFormatters: FieldFormatInstanceType[] = [ | |||
| BoolFormat, | |||
| BytesFormat, | |||
| ColorFormat, | |||
| DateNanosFormat, | |||
There was a problem hiding this comment.
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
tsullivan
commented
Jul 2, 2020
| * 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; |
There was a problem hiding this comment.
This section is copied from the date_server file
tsullivan
added
release_note:skip
…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.
tsullivan
force-pushed
the
data/allow-custom-formatting
branch
from
21cc35d to
1789afc
Compare
Merged
4 tasks
joelgriffith
approved these changes
Jul 7, 2020
|
@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. |
|
@tsullivan looks good to me! |
tsullivan
commented
Jul 8, 2020
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
tsullivan
commented
Jul 9, 2020
docs/development/plugins/data/server/kibana-plugin-plugins-data-server.fieldformats.md
Show resolved
Hide resolved
tsullivan
commented
Jul 9, 2020
|
|
||
| return this.getInstance(conf.id, conf.params); | ||
| return this.getInstance(conf.id, instanceParams); |
There was a problem hiding this comment.
This allows timezone to be passed from the browser and formatted correctly on the server
|
@elasticmachine merge upstream |
tsullivan
commented
Jul 13, 2020
src/plugins/data/server/field_formats/converters/date_nanos_server.ts
Outdated
Show resolved
Hide resolved
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
|
@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! |
mattkime
approved these changes
Jul 13, 2020
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Related to: https://github.com/elastic/kibana/pull/29977/files and adds the timezone formatting capability for the
date_serverfield 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:
DateNanosFormatfrom being a "common" field formatter to being 2 different formatters for browser and serversetCustomParamsto the FieldFormatsRegistry classFor 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