-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution][Resolver] Update @timestamp formatting #78166
Conversation
54496d2
to
41a19cc
Compare
Pinging @elastic/endpoint-app-team (Feature:Resolver) |
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( |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ doc comment needed
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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())) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, thanks
There was a problem hiding this comment.
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 null
s or something.
There was a problem hiding this comment.
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
const dateFormatSetting: string = useUiSetting('dateFormat'); | ||
const timezoneSetting: string = useUiSetting('dateFormat:tz'); | ||
|
||
if (timestamp === undefined) return invalidDateText; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ oooh fancy
4f7e4fc
to
714a6cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
35bd5d9
to
a5f336a
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
…8166) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* 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) ...
) (#78533) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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:
Node Detail:
Events Of Type:
Event Detail:
For maintainers