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

Generating CSV doesn’t include seconds and seconds fractions #114

Closed
Tracked by #162
zhongnansu opened this issue Jul 7, 2021 · 8 comments
Closed
Tracked by #162

Generating CSV doesn’t include seconds and seconds fractions #114

zhongnansu opened this issue Jul 7, 2021 · 8 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@zhongnansu
Copy link
Member

zhongnansu commented Jul 7, 2021

Is your feature request related to a problem? Please describe.
Currently for the date field in csv report. In the code there are 2 steps to handle date value.

  1. during data query stage, we added a format: date_hour_mintue as docValue(e.g. yyyy-MM-dd'T'HH:mm)
    https://github.com/opensearch-project/dashboards-reports/blob/e5174537800c60b1bf3145a80c4a36ab4227b80b/dashboards-reports/server/routes/utils/savedSearchReportHelper.ts#L270
  2. During csv rendering stage, we use moment.js to format it into 'MM/DD/YYYY h:mm:ss a', (e.g. 06/27/2021 9:59:00 pm)
    https://github.com/opensearch-project/dashboards-reports/blob/e5174537800c60b1bf3145a80c4a36ab4227b80b/dashboards-reports/server/routes/utils/dataReportHelpers.ts#L173-L175
    https://github.com/opensearch-project/dashboards-reports/blob/e5174537800c60b1bf3145a80c4a36ab4227b80b/dashboards-reports/server/routes/utils/constants.ts#L67

Notice is the first step above, we are cutting off the seconds and seconds fractions, comparing with what seems like the default date field format in advanced UI setting. And in step 2, the seconds fields will always be 00, becuase of the cut off in step 1.
image

Describe the solution you'd like
maybe cutting off seconds for date field is not a good choice. We have the following format as available options. Maybe we should use date_hour_mintue_seconds or date_hour_minute_second_fraction

Describe alternatives you've considered
Retrieve date format setting from Advanced UI setting and use that, but I feel like this will not only apply to date format, but also other settings, such as csv seperator, timezone, url prefix, etc. It's better to add it as a compete feature to support advanced UI setting loading

Additional context
This issue was originally raised from Opensearch forum https://discuss.opendistrocommunity.dev/t/generating-csv-doesnt-include-seconds-on-timestemp-fields/6413/6

@zhongnansu zhongnansu added enhancement New feature or request good first issue Good for newcomers labels Jul 7, 2021
@zhongnansu zhongnansu self-assigned this Oct 4, 2021
@joshuali925
Copy link
Member

We can use uiSettings.get('dateFormat') in frontend to get the date format in advanced settings.

The API request needs to be modified or a new route needs to be added to support all advanced settings (context menu and dashboards server doesn't have direct access to uiSettings). Also the default timeFormat MMM D, YYYY @ HH:mm:ss.SSS doesn't look very common. I'm thinking to just extend current time format to MM/DD/YYYY h:mm:ss.SSS a to fix this.

@zhongnansu
Copy link
Member Author

We can use uiSettings.get('dateFormat') in frontend to get the date format in advanced settings.

The API request needs to be modified or a new route needs to be added to support all advanced settings (context menu and dashboards server doesn't have direct access to uiSettings). Also the default timeFormat MMM D, YYYY @ HH:mm:ss.SSS doesn't look very common. I'm thinking to just extend current time format to MM/DD/YYYY h:mm:ss.SSS a to fix this.

@joshuali925
uiSettings are available in CoreStart as this PR suggests, is it doable in our case? So overall we want to make sure at least for values, what users see in Discover is the same as what they get in csv report.

@zhongnansu
Copy link
Member Author

zhongnansu commented Oct 20, 2021

@joshuali925 we can do this step by step, no need to support everyting in advanced settings at once. can start with date format

@joshuali925
Copy link
Member

@zhongnansu Yes this is doable, my concern is that 10/21/2021 6:55:21.748 am should be easier to parse and more standard than Oct 20, 2021 @ 23:55:21.748 in CSV in general. Also this way we don't introduce a big change to time fields in CSV.

But I'm not that familiar with csv processing tools, and if you feel it's ok to make the change then I can update the PR

@zhongnansu
Copy link
Member Author

@joshuali925 Found some reference. elastic/kibana#56153
Instead of using some predefined format, we'll need to move to use UI settings anyway

@kgcreative Hi Kevin, any thoughts?

@joshuali925
Copy link
Member

@zhongnansu Got it, then it's better to let users decide. I updated the PR

@kgcreative
Copy link
Member

kgcreative commented Oct 20, 2021

@joshuali925 @zhongnansu - for report settings, I think it makes sense to add a "date format" field under "time range"
(edit:)
image

An additional enhancement under the on-demand Generate CSV could be to make that a flyout with a date format option.

Date format should by default inherit user settings under advanced settings - but be overridable by the user, since different applications might expect the date format in different ways.

@zhongnansu
Copy link
Member Author

Moving the discussion here #208

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants