-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[APM UI] Refactor to use @timestamp
and make timestamp.us
optional as fallback value
#194100
base: main
Are you sure you want to change the base?
Conversation
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
*/ | ||
|
||
export function getTimestamp(timestamp?: string, fallbackTimestamp?: number): number { | ||
if (!fallbackTimestamp && timestamp) { |
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.
In case timestamp
and fallbackTimestamp
are both true
then fallbackTimestamp
is returned. I think it would be better to rely on timestamp
in that scenario, wdyt?
maybe an opposite condition?
if (!timestamp && fallbackTimestamp) {
return fallbackTimestamp;
}
return new Date(timestamp).getTime() * 1000 || 0
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 would make sense, let's see what others think and I can implement it if everyone agrees
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.
If we don't have timestamp.us, we return @timestamp ✅
Can we rename these variables? Because as per this comment ☝️ I was confused of which one is the fallback @timestamp
or timestamp_us
const timestamp = getTimestamp(errorData?.error?.['@timestamp'], errorData?.error?.timestamp?.us);
In this case fallback is timestamp_us
, but if we don't have it then the 'fallback' is @timestamp
I will keep the variables with the same name or close to the real field timestamp
& timestampUs
to avoid confusion
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.
changed to timestamp
and timestampUs
as the order of the params should always be the same
|
||
export function getTimestamp(timestamp?: string, fallbackTimestamp?: number): number { | ||
if (!fallbackTimestamp && timestamp) { | ||
return new Date(timestamp).getTime() * 1000; |
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 will truncate the microsecond precision when the timestamp is of type date_nanos
. Is there a way to retain the microsecond precision?
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 did a refactor to keep the microsecond precision
You can check if it contains nanoseconds by checking the number of digits in the fractional seconds (the part after the decimal point). Nanoseconds would be a 9-digit value after the decimal. function isNanoseconds(@timestamp) {
// Match the fractional seconds (digits after the dot) in the ISO string
const match = timestamp.match(/\.(\d+)Z$/);
if (match) {
const fractionalSeconds = match[1]; // The digits after the decimal point
return fractionalSeconds.length === 9;
}
return false;
} Note Haven't tried and I'm not super good with Regex, but something like this could work |
@@ -22,6 +22,7 @@ import { | |||
ERROR_LOG_MESSAGE, | |||
EVENT_OUTCOME, | |||
FAAS_COLDSTART, | |||
FALLBACK_TIMESTAMP, |
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.
Same in here
TIMESTAMP
& TIMESTAMP_US
, it's more clear IMHO
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.
fixed
@MiriamAparicio thanks for the suggestion, that could be a great way to check it! 👍 |
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 still think the Kibana server should handle all the logic and the UI should use what the server provided.
Today APM server provides the timestamp.us
which will be gone soon. But for the UI it doesn't matter who provided it.
We will have the @timestamp
field that is a string version and formatted as nanoseconds, and we will have the timestamp.us
which is the microseconds date extracted from either the current doc.timestamp.us
or by converting @timestamp
.
The server will still return the exact same thing, and no change would be needed on the client.
Another point is, by doing it on the server you'd convert the string to microseconds once and return it, while on the client you need to do it every single time you need to use the field. It's very easy for us to introduce bugs by forgetting to use the getTimestamp
function on the client.
When you say "server", do you mean the server-side component of Kibana? |
Yes, Kibana server side. |
Summary
Warning
This is still a WIP, I want to gather some feedback on the proposed solutions and explore other options.
Closes #191046
This PR aims to use
@timestamp
as the primary value, instead oftimestamp.us
as we have done until now.This change will be possible once we have
date_nanos
for@timestamp
, so we can get more precise dates.Also, we want to stop maintaining 2 timestamp values.
To make this change easy and backward-safe, I created a shared function that will get the timestamp, following the criteria of the parent issue:
timestamp.us
, we return@timestamp
✅@timestamp
isdate_nanos
, prefer it overtimestamp.us
❌Some drawbacks I've encountered:
long
fortimestamp.us
andstring
for@timestamp
. Something we need to adapt when working with them, as right now the UI depends ontimestamp.us
Open questions:
@timestamp
isdate_nanos
on the client?