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

[Security Solution][Resolver] Update @timestamp formatting #78166

Merged
merged 9 commits into from
Sep 25, 2020

Conversation

michaelolo24
Copy link
Contributor

@michaelolo24 michaelolo24 commented Sep 22, 2020

Summary

Updates the timestamp fields in the panel to use the dateFormat defined in the advanced settings. See below for the default format:

Node List:

Screen Shot 2020-09-22 at 3 35 25 PM

Node Detail:

Screen Shot 2020-09-22 at 3 35 49 PM

Events Of Type:

Screen Shot 2020-09-22 at 3 37 44 PM

Event Detail:

Screen Shot 2020-09-22 at 3 40 57 PM

For maintainers

@michaelolo24 michaelolo24 added Feature:Resolver Security Solution Resolver feature Team:Endpoint Data Visibility Team managing the endpoint resolver v7.10.0 v8.0.0 labels Sep 22, 2020
@michaelolo24 michaelolo24 marked this pull request as ready for review September 22, 2020 18:47
@michaelolo24 michaelolo24 requested review from a team as code owners September 22, 2020 18:47
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Resolver)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

import { useUiSetting } from '../../../../../../../src/plugins/kibana_react/public';
import { formatter, invalidDateText } from './panel_content_utilities';

export function useDateFormatSetting(
Copy link
Contributor

Choose a reason for hiding this comment

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

This hook returns a formatted date so maybe this name could be useFormattedDate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, just renamed it. Saw I missed a few spots. Was confused why the ticket only named the event-detail, but it's all timestamps in the panel. Anyways will be updated shortly


import moment from 'moment-timezone';
import { useUiSetting } from '../../../../../../../src/plugins/kibana_react/public';
import { formatter, invalidDateText } from './panel_content_utilities';
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just inline these values here, they probably shouldn't need to be in 'panel_content_utilities'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, just did that. Will be up shortly

@@ -122,7 +122,7 @@ export const formatter = new Intl.DateTimeFormat(i18n.getLocale(), {
second: '2-digit',
});

const invalidDateText = i18n.translate(
export const invalidDateText = i18n.translate(
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ doc comment needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should export string literals like this. Let's just move it to where it is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No exporting anymore, but will add

if (timestamp === undefined) return null;

const date = new Date(timestamp);
if (date && isFinite(date.getTime())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Should this be Number.isFinite instead of the global one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

That wasn't a passive-aggressive "I think that should be Number.isFinite" btw, I seriously don't know. It might depend on if you want to allow nulls or something.

Copy link
Contributor Author

@michaelolo24 michaelolo24 Sep 22, 2020

Choose a reason for hiding this comment

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

Oh lol, I didn't take it as passive aggressive 😂, it just should be since we cast it to a date beforehand so getTime() should return a number if it's a valid date. Just realized I didn't update this either. Will update the PR

@michaelolo24 michaelolo24 added the release_note:skip Skip the PR/issue when compiling release notes label Sep 22, 2020
const dateFormatSetting: string = useUiSetting('dateFormat');
const timezoneSetting: string = useUiSetting('dateFormat:tz');

if (timestamp === undefined) return invalidDateText;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to list the lack of a timestamp as an invalidDate or just leave it blank and return and empty string? There were some places where we did that, just wasn't sure if that would be confusing to the user. Maybe text here instead saying Missing Time Details or similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

ya, we could use the missing badge that we use somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where can one find said badge?

Copy link
Contributor

@kqualters-elastic kqualters-elastic left a comment

Choose a reason for hiding this comment

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

lgtm

/**
*
* @description formats a given time based on the user defined format in the advanced settings section of kibana under dateFormat
* @export
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ oooh fancy

@michaelolo24 michaelolo24 force-pushed the update-timestamp-formatting branch 3 times, most recently from 4f7e4fc to 714a6cd Compare September 23, 2020 19:28
Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

nice

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 1968 +1 1967

async chunks size

id value diff baseline
securitySolution 10.2MB +2.0KB 10.2MB

History

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

@michaelolo24 michaelolo24 merged commit 82ceb87 into elastic:master Sep 25, 2020
@michaelolo24 michaelolo24 deleted the update-timestamp-formatting branch September 25, 2020 12:33
michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Sep 25, 2020
…8166)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 28, 2020
* master: (226 commits)
  [Enterprise Search] Added Logic for the Credentials View (elastic#77626)
  [CSM] Js errors (elastic#77919)
  Add the @kbn/apm-config-loader package (elastic#77855)
  [Security Solution] Refactor useSelector (elastic#75297)
  Implement tagcloud renderer (elastic#77910)
  [APM] Alerting: Add global option to create all alert types (elastic#78151)
  [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939)
  TypeScript cleanup in visualizations plugin (elastic#78428)
  Lazy load metric & mardown visualizations (elastic#78391)
  [Detections][EQL] EQL rule execution in detection engine (elastic#77419)
  Update tutorial-full-experience.asciidoc (elastic#75836)
  Update tutorial-define-index.asciidoc (elastic#75754)
  Add support for runtime field types to mappings editor. (elastic#77420)
  [Monitoring] Usage collection (elastic#75878)
  [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316)
  [Security Solution][Resolver] Update @timestamp formatting (elastic#78166)
  [Security Solution] Fix app layout (elastic#76668)
  [Security Solution][Resolver] 2 new functions to DAL (elastic#78477)
  Adds new elasticsearch client to telemetry plugin (elastic#78046)
  skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500)
  ...
michaelolo24 added a commit that referenced this pull request Sep 29, 2020
) (#78533)

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
Feature:Resolver Security Solution Resolver feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants