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

Send logdna now argument in milliseconds #23

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

slepic
Copy link
Contributor

@slepic slepic commented Feb 5, 2024

As discussedi in #21 this implementation takes the now value from system time and sends it to logdna in milliseonds as per documentation (https://docs.mezmo.com/log-analysis-api#ingest)

I also added the possibility to turn the time drift calculation off by not sending the now parameter

$handler->setIncludeRequestTime(false);

@@ -75,6 +80,11 @@ public function setTags($tags)
$this->tags = $tags;
}

public function setIncludeRequestTime($include)
Copy link
Owner

Choose a reason for hiding this comment

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

Missing @param documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@@ -128,4 +143,11 @@ protected function getDefaultFormatter(): FormatterInterface
{
return new \Zwijn\Monolog\Formatter\LogdnaFormatter();
}

private function getCurrentTimeInMilliseconds(): string
Copy link
Owner

Choose a reason for hiding this comment

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

I would just remove this function or replace it's content with

return (string)floor(microtime(true) * 1000);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -108,6 +118,11 @@ protected function write(\Monolog\LogRecord $record): void
'ip' => $this->ip,
'tags' => $this->tags
];

if ($this->include_request_time) {
$query['now'] = $this->getCurrentTimeInMilliseconds();
Copy link
Owner

Choose a reason for hiding this comment

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

We could replace this with just (string)floor(microtime(true) * 1000).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@slepic
Copy link
Contributor Author

slepic commented Feb 6, 2024

I was originally concerned that floating point arithmetics might not be suitable here, but

  1. I couldn't find any particular timestamp for which it would break (well i didnt try all the trillions of them...)
  2. According to wiki - double precision floating point format numbers should grant 15-17 significant digits. The current timestamp in milliseconds needs 13 and we have about 263 years until 14th digit is needed.
  3. single precision floating point numbers however have only 6-9 significant digits. This would only apply on machines that do not implement double precision floating point arithmetics in their CPU and I'm totally fine ignoring that possibility these days.

@nvanheuverzwijn nvanheuverzwijn merged commit f60a9ef into nvanheuverzwijn:master Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants