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

Render PPL time column using the correct time zone #9379

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

abbyhu2000
Copy link
Member

Description

When sending PPL requests, the time based fields are rendered in wrong time zone, and it looks like DQL and PPL are getting different results. We should format the PPL search response to the same format as DQL search response to indicate it is in UTC so the time fields can be formatted and rendered correctly in the discover table according to user defined time zone in advanced setting.

For example, previously

// PPL search response
"order_date": "2025-02-13 00:51:50"

// DQL search response
"order_date": "2025-02-13T00:51:50+00:00”

Issues Resolved

resolves #9104

Screenshot

Before:

PPL render time field in the wrong timezone

Screen.Recording.2025-02-12.at.5.22.01.PM.mov

After:

PPL and DQL return and render same results for default timezone or user defined timezone

Screen.Recording.2025-02-12.at.5.17.57.PM.mov

Testing the changes

Changelog

  • fix: Make PPL time column respect time zone and date format

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
@abbyhu2000 abbyhu2000 changed the title Make PPL time column respect time zone and date format Render PPL time column using the correct time zone Feb 13, 2025
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.74%. Comparing base (8cc5f84) to head (abbdd62).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9379      +/-   ##
==========================================
+ Coverage   61.71%   61.74%   +0.03%     
==========================================
  Files        3817     3817              
  Lines       91841    91853      +12     
  Branches    14545    14550       +5     
==========================================
+ Hits        56677    56715      +38     
+ Misses      31508    31481      -27     
- Partials     3656     3657       +1     
Flag Coverage Δ
Linux_1 28.98% <0.00%> (-0.01%) ⬇️
Linux_2 56.44% <0.00%> (-0.02%) ⬇️
Linux_4 28.89% <0.00%> (-0.01%) ⬇️
Windows_1 28.99% <0.00%> (-0.01%) ⬇️
Windows_2 56.39% <0.00%> (-0.02%) ⬇️
Windows_3 39.25% <100.00%> (+0.05%) ⬆️
Windows_4 28.89% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Maosaic
Copy link
Contributor

Maosaic commented Feb 13, 2025

Any unit test we want to add or update?


const processField = (field: any, value: any): any => {
// Handle date fields
if (moment(value, ['YYYY-MM-DD HH:mm:ss'], true).isValid()) {
Copy link
Member

@kavilla kavilla Feb 13, 2025

Choose a reason for hiding this comment

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

i can't remember this is before we know it is a date field correct? or do we already know it's a date field? as in field.type === 'date'.

YYYY-MM-DD HH:mm:ss seems too specific to PPL, correct me if im wrong.

would it make sense to do it something like

const tzConfig = opensearchDashboards.services.uiSettings!.get(UI_SETTINGS.DATE_FORMAT_TIMEZONE);
const momentParsedValue = moment(value).tz(tzConfig);
if (momentParsedValue.isValid()) return momentParsedValue?.format('YYYY-MM-DDTHH:mm:ss.SSSZ');

or does it need to be +00:00 because to me it feels like we would be forcing any date value of YYYY-MM-DD HH:mm:ss to have a value we statically append +00:00

Copy link
Member Author

@abbyhu2000 abbyhu2000 Feb 14, 2025

Choose a reason for hiding this comment

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

yea at first i used field.type === 'date' but then i realized this will skip those nested field which does not have a field.type yet, therefore i switch to use moment.

Copy link
Member Author

@abbyhu2000 abbyhu2000 Feb 14, 2025

Choose a reason for hiding this comment

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

We do not need to get or change the timezone here because opensearch backend always store and return time value as UTC, no matter which timezone we define in the advanced setting. And then the table formatter will reformat all the values according to the advanced setting later. So here we should not change time zone.

We could potentially do:

if (moment.utc(value).isValid())  or if(moment(value).isValid())

but this don't work for some reason: when the value type is object, it will always go into the if loop as it is a date. I tried multiple methods for determining if a string is a date, so far only if (moment(value, ['YYYY-MM-DD HH:mm:ss'], true).isValid()) does the correct job. We could potentially do if (moment(value, ['YYYY-MM-DD HH:mm:ss', 'YYYY-MM-DD HH:mm:ss.SSS'], true).isValid()) to include more types here? i know only PPL doesn't include miliseconds

Copy link
Member Author

Choose a reason for hiding this comment

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

i changed return value.replace(' ', 'T') + '+00:00'; to return moment.utc(value).format('YYYY-MM-DDTHH:mm:ssZ'); so it it more general

@kavilla
Copy link
Member

kavilla commented Feb 13, 2025

wow. great find!

do we have a manual test or could we add some cypress/unit test for this one. when first implementing this extensibility i had completely neglected the responses could be coming back in a different timezone format. if developers wanted to add new langauges and new data sources it might be useful for them to ensure they properly are formatting their responses or that we do it for all cases. or at least documented

@abbyhu2000
Copy link
Member Author

Added unit test for convertResult() function @Maosaic @kavilla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Invalid handling of time filter in PPL
3 participants