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

Timestamp documentation not aligned with LogRecord implementation #4309

Closed
nckrse opened this issue Nov 18, 2023 · 4 comments
Closed

Timestamp documentation not aligned with LogRecord implementation #4309

nckrse opened this issue Nov 18, 2023 · 4 comments
Labels
bug Something isn't working document Documentation-related

Comments

@nckrse
Copy link

nckrse commented Nov 18, 2023

What happened?

Following documentation for log record syntax specifies using nanosecond epoch for timestamp, which results in error.

Steps to Reproduce

Use nanosecond value for timestamp in LogRecord Interface as documented (1, 2)

Expected Result

Resolved hrTime timestamp in emitted log

Actual Result

Logs fail to export OTLPExporterError: Bad Request

{"stack":"OTLPExporterError: Bad Request\n    at IncomingMessage.<anonymous> (/home/node/app/node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/node/util.js:103:39)\n    at IncomingMessage.emit (node:events:529:35)\n    at endReadableNT (node:internal/streams/readable:1368:12)\n    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)","message":"Bad Request","name":"OTLPExporterError","data":"{\"code\":3,\"message\":\"ReadUint64: strconv.ParseUint: parsing \\\"1700344572441000936000000\\\": value out of range, error found in #10 byte of ...|936000000\\\",\\\"observed|..., bigger context ...|ords\\\":[{\\\"timeUnixNano\\\":\\\"1700344572441000936000000\\\",\\\"observedTimeUnixNano\\\":\\\"1700344789042000000\\\",\\\"bod|...\"}","code":"400"}

Additional Details

After looking over the code, I found that millisecond epoch is the supplied input. Looking at the conversion, it looks like milli OR hrTime is supported, but not vanilla nanoseconds.

The documentation (1, 2):
The time when the log record occurred as UNIX Epoch time in nanoseconds. should probably be updated to reflect? Perhaps:
The time when the log record occurred as UNIX Epoch time in milliseconds or node hrTime.

Workaround:

  • Use millisecond epoch for timestamp

OpenTelemetry Setup Code

No response

package.json

No response

Relevant log output

{"stack":"OTLPExporterError: Bad Request\n    at IncomingMessage.<anonymous> (/home/node/app/node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/node/util.js:103:39)\n    at IncomingMessage.emit (node:events:529:35)\n    at endReadableNT (node:internal/streams/readable:1368:12)\n    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)","message":"Bad Request","name":"OTLPExporterError","data":"{\"code\":3,\"message\":\"ReadUint64: strconv.ParseUint: parsing \\\"1700344572441000936000000\\\": value out of range, error found in #10 byte of ...|936000000\\\",\\\"observed|..., bigger context ...|ords\\\":[{\\\"timeUnixNano\\\":\\\"1700344572441000936000000\\\",\\\"observedTimeUnixNano\\\":\\\"1700344789042000000\\\",\\\"bod|...\"}","code":"400"}
@nckrse nckrse added bug Something isn't working triage labels Nov 18, 2023
@dyladan
Copy link
Member

dyladan commented Nov 20, 2023

This is a documentation issue. The HrTime interface actually is nanoseconds. The issue is that a plain number using nanoseconds is too large to be stored safely as a JS number (all numbers are IEEE 754 floats) and causes inexact times. The HrTime interface splits the whole and partial number parts, avoiding this issue.

@dyladan dyladan added document Documentation-related and removed triage labels Nov 20, 2023
@seemk
Copy link
Contributor

seemk commented Dec 3, 2023

Perhaps timestamp field should be of type TimeInput (or at least HrTime) instead? In the logs spec, both timestamp and observedTimestamp are nanoseconds since unix epoch

@seemk
Copy link
Contributor

seemk commented Dec 3, 2023

Don't think there's a bug here, but in any case I opened a PR to allow for TimeInput to be passed in for LogRecord timestamps: #4345

@dyladan
Copy link
Member

dyladan commented Dec 6, 2023

Fixed by #4345

@dyladan dyladan closed this as completed Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working document Documentation-related
Projects
None yet
Development

No branches or pull requests

3 participants