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

[APM UI] Refactor to use @timestamp and make timestamp.us optional as fallback value #194100

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rmyz
Copy link
Contributor

@rmyz rmyz commented Sep 26, 2024

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 of timestamp.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:

  • If we don't have timestamp.us, we return @timestamp
  • (Optional) If @timestamp is date_nanos, prefer it over timestamp.us

Some drawbacks I've encountered:

  • Both values use different formats, long for timestamp.us and string for @timestamp. Something we need to adapt when working with them, as right now the UI depends on timestamp.us
  • Javascript doesn't support nanoseconds 64 bit number/integer support in Kibana #40183

Open questions:

  • How can we know if @timestamp is date_nanos on the client?

@elasticmachine
Copy link
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!

@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

*/

export function getTimestamp(timestamp?: string, fallbackTimestamp?: number): number {
if (!fallbackTimestamp && timestamp) {
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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;
Copy link
Member

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?

Copy link
Contributor Author

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

@rmyz rmyz self-assigned this Sep 26, 2024
@rmyz rmyz added apm apm:opentelemetry APM UI - OTEL Work Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team ci:project-deploy-observability Create an Observability project backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed apm backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Sep 26, 2024
@MiriamAparicio
Copy link
Contributor

How can we know if @timestamp is date_nanos on the client?

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. @timestamp would be an ISO string normally.

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,
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@rmyz
Copy link
Contributor Author

rmyz commented Sep 26, 2024

How can we know if @timestamp is date_nanos on the client?

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. @timestamp would be an ISO string normally.

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

@MiriamAparicio thanks for the suggestion, that could be a great way to check it! 👍
Anyway, I want to know if we will receive nanoseconds or microseconds before implementing it.
We can talk about this on Otel daily

Copy link
Contributor

@cauemarcondes cauemarcondes left a 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.

@felixbarny
Copy link
Member

When you say "server", do you mean the server-side component of Kibana?

@cauemarcondes
Copy link
Contributor

When you say "server", do you mean the server-side component of Kibana?

Yes, Kibana server side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:opentelemetry APM UI - OTEL Work ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Make timestamp.us optional and fall back to @timestamp
7 participants