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

refactor!: convert all SDK timestamps from HrTime to bigint #5522

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2c6b922
refactor!: convert all SDK timestamps from HrTime to bigint
dyladan Mar 4, 2025
7ef4dbd
Changelog
dyladan Mar 4, 2025
c4cb812
Lint
dyladan Mar 4, 2025
addb504
Fix conversion
dyladan Mar 5, 2025
19e1224
Fix zipkin tests
dyladan Mar 5, 2025
641f56f
Lint
dyladan Mar 5, 2025
989479a
Update packages/opentelemetry-core/src/common/time.ts
dyladan Mar 5, 2025
fb2c16c
Merge branch 'main' into hrtime-to-bigint
dyladan Mar 5, 2025
460d81d
Add logs to test
dyladan Mar 5, 2025
b5a32cd
Log start time too
dyladan Mar 5, 2025
d2c1506
Remove deep paths
dyladan Mar 6, 2025
9ed865f
log inconsistent times
dyladan Mar 6, 2025
6af9780
Gratuitous logging
dyladan Mar 6, 2025
7635a74
Fix time conversion
dyladan Mar 6, 2025
c2ca60e
Remove logging stuff
dyladan Mar 6, 2025
1d040bf
Use bigint in test case
dyladan Mar 6, 2025
ed25018
Fix browser tests
dyladan Mar 6, 2025
042f294
Add backwards compatible getters to metrics timestamps
dyladan Mar 6, 2025
0053b42
Add backwards compatible getters to logs timestamps
dyladan Mar 6, 2025
335367f
Revert messed up browser tests
dyladan Mar 6, 2025
ca0a17e
Lint
dyladan Mar 6, 2025
618dd6b
Merge branch 'main' into hrtime-to-bigint
dyladan Mar 6, 2025
eb5dadb
Remove param comment
dyladan Mar 7, 2025
8934944
Merge branch 'main' into hrtime-to-bigint
dyladan Mar 11, 2025
283c521
Undo API change
dyladan Mar 11, 2025
319ec97
Naming nit
dyladan Mar 11, 2025
7090018
Fix compile
dyladan Mar 11, 2025
a9b53e4
Remove obsolete test
dyladan Mar 11, 2025
3f01070
Lint
dyladan Mar 11, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ export class PrometheusSerializer {

name = enforcePrometheusNamingConvention(name, data);
const { value, attributes } = dataPoint;
const timestamp = nanosecondsToMilliseconds(dataPoint.endTime);
const timestamp = nanosecondsToMilliseconds(dataPoint.endTimeUnixNano);
results += stringify(
name,
attributes,
Expand All @@ -303,7 +303,7 @@ export class PrometheusSerializer {
name = enforcePrometheusNamingConvention(name, data);
const attributes = dataPoint.attributes;
const histogram = dataPoint.value;
const timestamp = nanosecondsToMilliseconds(dataPoint.endTime);
const timestamp = nanosecondsToMilliseconds(dataPoint.endTimeUnixNano);
/** Histogram["bucket"] is not typed with `number` */
for (const key of ['count', 'sum'] as ('count' | 'sum')[]) {
const value = histogram[key];
Expand Down
12 changes: 6 additions & 6 deletions experimental/packages/otlp-transformer/src/metrics/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,8 @@ function toSingularDataPoint(
) {
const out: INumberDataPoint = {
attributes: toAttributes(dataPoint.attributes),
startTimeUnixNano: encoder.encodeBigIntNanos(dataPoint.startTime),
timeUnixNano: encoder.encodeBigIntNanos(dataPoint.endTime),
startTimeUnixNano: encoder.encodeBigIntNanos(dataPoint.startTimeUnixNano),
timeUnixNano: encoder.encodeBigIntNanos(dataPoint.endTimeUnixNano),
};

switch (valueType) {
Expand Down Expand Up @@ -161,8 +161,8 @@ function toHistogramDataPoints(
sum: histogram.sum,
min: histogram.min,
max: histogram.max,
startTimeUnixNano: encoder.encodeBigIntNanos(dataPoint.startTime),
timeUnixNano: encoder.encodeBigIntNanos(dataPoint.endTime),
startTimeUnixNano: encoder.encodeBigIntNanos(dataPoint.startTimeUnixNano),
timeUnixNano: encoder.encodeBigIntNanos(dataPoint.endTimeUnixNano),
};
});
}
Expand All @@ -189,8 +189,8 @@ function toExponentialHistogramDataPoints(
},
scale: histogram.scale,
zeroCount: histogram.zeroCount,
startTimeUnixNano: encoder.encodeBigIntNanos(dataPoint.startTime),
timeUnixNano: encoder.encodeBigIntNanos(dataPoint.endTime),
startTimeUnixNano: encoder.encodeBigIntNanos(dataPoint.startTimeUnixNano),
timeUnixNano: encoder.encodeBigIntNanos(dataPoint.endTimeUnixNano),
};
});
}
Expand Down
6 changes: 5 additions & 1 deletion experimental/packages/otlp-transformer/test/logs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { TraceFlags } from '@opentelemetry/api';
import { HrTime, TraceFlags } from '@opentelemetry/api';
import { InstrumentationScope } from '@opentelemetry/core';
import { Resource, resourceFromAttributes } from '@opentelemetry/resources';
import * as assert from 'assert';
Expand Down Expand Up @@ -183,6 +183,8 @@ describe('Logs', () => {
const log_fragment_1 = {
timeUnixNano: 1680253513123241635n,
timeUnixNanoObserved: 1683526948965142784n,
hrTime: [1680253513, 123241635] as HrTime,
hrTimeObserved: [1683526948, 965142784] as HrTime,
attributes: {
'some-attribute': 'some attribute value',
},
Expand All @@ -199,6 +201,8 @@ describe('Logs', () => {
const log_fragment_2 = {
timeUnixNano: 1680253797687038506n,
timeUnixNanoObserved: 1680253797687038506n,
hrTime: [1680253797, 687038506] as HrTime,
hrTimeObserved: [1680253797, 687038506] as HrTime,
attributes: {
'another-attribute': 'another attribute value',
},
Expand Down
42 changes: 28 additions & 14 deletions experimental/packages/otlp-transformer/test/metrics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,10 @@ describe('Metrics', () => {
dataPoints: [
{
value: value,
startTime: START_TIME,
endTime: END_TIME,
startTimeUnixNano: START_TIME,
endTimeUnixNano: END_TIME,
startTime: [0, 0],
endTime: [0, 0],
attributes: ATTRIBUTES,
},
],
Expand All @@ -147,8 +149,10 @@ describe('Metrics', () => {
dataPoints: [
{
value: value,
startTime: START_TIME,
endTime: END_TIME,
startTimeUnixNano: START_TIME,
endTimeUnixNano: END_TIME,
startTime: [0, 0],
endTime: [0, 0],
attributes: ATTRIBUTES,
},
],
Expand All @@ -172,8 +176,10 @@ describe('Metrics', () => {
dataPoints: [
{
value: value,
startTime: START_TIME,
endTime: END_TIME,
startTimeUnixNano: START_TIME,
endTimeUnixNano: END_TIME,
startTime: [0, 0],
endTime: [0, 0],
attributes: ATTRIBUTES,
},
],
Expand All @@ -197,8 +203,10 @@ describe('Metrics', () => {
dataPoints: [
{
value: value,
startTime: START_TIME,
endTime: END_TIME,
startTimeUnixNano: START_TIME,
endTimeUnixNano: END_TIME,
startTime: [0, 0],
endTime: [0, 0],
attributes: ATTRIBUTES,
},
],
Expand All @@ -218,8 +226,10 @@ describe('Metrics', () => {
dataPoints: [
{
value: value,
startTime: START_TIME,
endTime: END_TIME,
startTimeUnixNano: START_TIME,
endTimeUnixNano: END_TIME,
startTime: [0, 0],
endTime: [0, 0],
attributes: ATTRIBUTES,
},
],
Expand Down Expand Up @@ -256,8 +266,10 @@ describe('Metrics', () => {
counts: counts,
},
},
startTime: START_TIME,
endTime: END_TIME,
startTimeUnixNano: START_TIME,
endTimeUnixNano: END_TIME,
startTime: [0, 0],
endTime: [0, 0],
attributes: ATTRIBUTES,
},
],
Expand Down Expand Up @@ -296,8 +308,10 @@ describe('Metrics', () => {
positive: positive,
negative: negative,
},
startTime: START_TIME,
endTime: END_TIME,
startTimeUnixNano: START_TIME,
endTimeUnixNano: END_TIME,
startTime: [0, 0],
endTime: [0, 0],
attributes: ATTRIBUTES,
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ describe('Trace', () => {
{
name: 'some event',
timeUnixNano: 1640715558542725388n,
time: [0, 0],
attributes: {
'event-attribute': 'some string value',
},
Expand Down
17 changes: 15 additions & 2 deletions experimental/packages/sdk-logs/src/LogRecord.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,19 @@
import { AttributeValue, diag } from '@opentelemetry/api';
import type * as logsAPI from '@opentelemetry/api-logs';
import * as api from '@opentelemetry/api';
import { isAttributeValue, InstrumentationScope } from '@opentelemetry/core';
import {
isAttributeValue,
InstrumentationScope,
nanosToHrTime,
} from '@opentelemetry/core';
import type { Resource } from '@opentelemetry/resources';

import type { ReadableLogRecord } from './export/ReadableLogRecord';
import type { LogRecordLimits } from './types';
import { AnyValue, LogAttributes, LogBody } from '@opentelemetry/api-logs';
import { LoggerProviderSharedState } from './internal/LoggerProviderSharedState';
import { timeInputToNano } from '@opentelemetry/core/src/common/time';
import { timeInputToNano } from '@opentelemetry/core';
import { HrTime } from '@opentelemetry/api';

export class LogRecord implements ReadableLogRecord {
readonly timeUnixNano: bigint;
Expand Down Expand Up @@ -75,6 +80,14 @@
return this.totalAttributesCount - Object.keys(this.attributes).length;
}

get hrTime(): HrTime {
return nanosToHrTime(this.timeUnixNano);

Check warning on line 84 in experimental/packages/sdk-logs/src/LogRecord.ts

View check run for this annotation

Codecov / codecov/patch

experimental/packages/sdk-logs/src/LogRecord.ts#L83-L84

Added lines #L83 - L84 were not covered by tests
}

get hrTimeObserved(): HrTime {
return nanosToHrTime(this.timeUnixNanoObserved);

Check warning on line 88 in experimental/packages/sdk-logs/src/LogRecord.ts

View check run for this annotation

Codecov / codecov/patch

experimental/packages/sdk-logs/src/LogRecord.ts#L87-L88

Added lines #L87 - L88 were not covered by tests
}

constructor(
_sharedState: LoggerProviderSharedState,
instrumentationScope: InstrumentationScope,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import type { Resource } from '@opentelemetry/resources';
import type { SpanContext } from '@opentelemetry/api';
import type { HrTime, SpanContext } from '@opentelemetry/api';
import type { InstrumentationScope } from '@opentelemetry/core';
import type {
LogBody,
Expand All @@ -26,6 +26,10 @@ import type {
export interface ReadableLogRecord {
readonly timeUnixNano: bigint;
readonly timeUnixNanoObserved: bigint;
/** @deprecated please use timeUnixNano */
readonly hrTime: HrTime;
/** @deprecated please use timeUnixNanoObserved */
readonly hrTimeObserved: HrTime;
readonly spanContext?: SpanContext;
readonly severityText?: string;
readonly severityNumber?: SeverityNumber;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import {
millisecondsToNanoseconds,
timeInputToNano,
getTimeOrigin,
} from '@opentelemetry/core/src/common/time';
} from '@opentelemetry/core';

const setup = (logRecordLimits?: LogRecordLimits, data?: logsAPI.LogRecord) => {
const instrumentationScope = {
Expand Down
18 changes: 14 additions & 4 deletions experimental/packages/shim-opencensus/src/metric-transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import * as oc from '@opencensus/core';
import { Attributes, ValueType, diag } from '@opentelemetry/api';
import { Attributes, HrTime, ValueType, diag } from '@opentelemetry/api';
import { hrTimeToNanoseconds } from '@opentelemetry/core';
import {
AggregationTemporality,
Expand Down Expand Up @@ -169,24 +169,34 @@
const attributes = zipOcLabels(metric.descriptor.labelKeys, ts.labelValues);

// use zeroed hrTime if it is undefined, which probably shouldn't happen
const startTime = ocTimestampToHrTime(ts.startTimestamp) ?? 0n;
const startTimeUnixNano = ocTimestampToNanos(ts.startTimestamp) ?? 0n;
const startTime = ocTimestampToHrTime(ts.startTimestamp) ?? [0, 0];

// points should be an array with a single value, so this will return a single point per
// attribute set.
return ts.points.map(
(point): DataPoint<T> => ({
startTimeUnixNano,
startTime,
attributes,
value: valueMapper(point.value),
endTime: ocTimestampToHrTime(point.timestamp) ?? 0n,
endTimeUnixNano: ocTimestampToNanos(point.timestamp) ?? 0n,
endTime: ocTimestampToHrTime(point.timestamp) ?? [0, 0],
})
);
});
}

function ocTimestampToHrTime(ts: oc.Timestamp | undefined): bigint | null {
function ocTimestampToHrTime(ts: oc.Timestamp | undefined): HrTime | null {
if (ts === undefined || ts.seconds === null) {
return null;
}
return [ts.seconds, ts.nanos ?? 0];
}

function ocTimestampToNanos(ts: oc.Timestamp | undefined): bigint | null {
if (ts === undefined || ts.seconds === null) {
return null;

Check warning on line 199 in experimental/packages/shim-opencensus/src/metric-transform.ts

View check run for this annotation

Codecov / codecov/patch

experimental/packages/shim-opencensus/src/metric-transform.ts#L199

Added line #L199 was not covered by tests
}
return hrTimeToNanoseconds([ts.seconds, ts.nanos ?? 0]);
}
Expand Down
4 changes: 2 additions & 2 deletions experimental/packages/shim-opencensus/test/ShimSpan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('ShimSpan', () => {
});

assert.strictEqual(span.events.length, 1);
const [{ timeUnixNano, ...event }] = span.events;
const [{ timeUnixNano, time, ...event }] = span.events;
assert.deepStrictEqual(event, {
attributes: {
foo: 'bar',
Expand All @@ -85,7 +85,7 @@ describe('ShimSpan', () => {
});

assert.strictEqual(span.events.length, 1);
const [{ timeUnixNano, ...event }] = span.events;
const [{ timeUnixNano, time, ...event }] = span.events;
assert.deepStrictEqual(event, {
attributes: {
'message.event.size.compressed': 15,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,10 @@ describe('metric-transform', () => {
dataPoints: [
{
attributes: { key1: 'value1', key2: 'value2' },
endTime: 20_000_000_020n,
startTime: 10_000_000_010n,
endTimeUnixNano: 20_000_000_020n,
startTimeUnixNano: 10_000_000_010n,
startTime: [10, 10],
endTime: [20, 20],
value: 5,
},
],
Expand Down Expand Up @@ -97,8 +99,10 @@ describe('metric-transform', () => {
dataPoints: [
{
attributes: { key1: 'value1', key2: 'value2' },
endTime: 20_000_000_020n,
startTime: 10_000_000_010n,
endTimeUnixNano: 20_000_000_020n,
startTimeUnixNano: 10_000_000_010n,
startTime: [10, 10],
endTime: [20, 20],
value: 5.5,
},
],
Expand Down Expand Up @@ -159,8 +163,10 @@ describe('metric-transform', () => {
dataPoints: [
{
attributes: { key1: 'value1', key2: 'value2' },
endTime: 20_000_000_020n,
startTime: 10_000_000_010n,
endTimeUnixNano: 20_000_000_020n,
startTimeUnixNano: 10_000_000_010n,
startTime: [10, 10],
endTime: [20, 20],
value: {
buckets: {
boundaries: [1, 10, 100],
Expand Down Expand Up @@ -207,8 +213,10 @@ describe('metric-transform', () => {
dataPoints: [
{
attributes: { key1: 'value1', key2: 'value2' },
endTime: 20_000_000_020n,
startTime: 10_000_000_010n,
endTimeUnixNano: 20_000_000_020n,
startTimeUnixNano: 10_000_000_010n,
startTime: [10, 10],
endTime: [20, 20],
value: 5,
},
],
Expand Down Expand Up @@ -248,8 +256,10 @@ describe('metric-transform', () => {
dataPoints: [
{
attributes: { key1: 'value1', key2: 'value2' },
endTime: 20_000_000_020n,
startTime: 10_000_000_010n,
endTimeUnixNano: 20_000_000_020n,
startTimeUnixNano: 10_000_000_010n,
startTime: [10, 10],
endTime: [20, 20],
value: 5.5,
},
],
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-core/src/common/time.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,18 +142,18 @@
return BigInt(millis) * 1_000_000n;
} else {
const out =
BigInt(Math.round(millis)) * 1000000n +
BigInt(Math.trunc(millis)) * 1000000n +
BigInt(Math.round((millis % 1) * 1000000));
return out;
}
}

export function nanosecondsToMilliseconds(nanos: bigint): number {
return Number(nanos / 1_000_000n);

Check warning on line 152 in packages/opentelemetry-core/src/common/time.ts

View check run for this annotation

Codecov / codecov/patch

packages/opentelemetry-core/src/common/time.ts#L152

Added line #L152 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

This impl (and less so the impl for nanosecondsToMicroseconds below) loses precision: the fractional-milliseconds get dropped.

> t = core.hrTime()
[ 1741582778, 35610255 ]

> core.hrTimeToMilliseconds(t)   # converted directly to millis
1741582778035.6104

# Converting to millis from nanos
> tNanos = core.hrTimeToNanoseconds(t)
1741582778035610255n
> core.nanosecondsToMilliseconds(tNanos)
1741582778035

I think this would work:

Suggested change
return Number(nanos / 1_000_000n);
return Number(nanos / 1_000_000n) + (Number(nanos % 1_000_000n) / 1_000_000)

}

export function nanosecondsToMicroseconds(nanos: bigint): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above, this loses the fractional microseconds.

return Number(nanos / 1_000n);

Check warning on line 156 in packages/opentelemetry-core/src/common/time.ts

View check run for this annotation

Codecov / codecov/patch

packages/opentelemetry-core/src/common/time.ts#L156

Added line #L156 was not covered by tests
}

export function nanosToHrTime(nanos: bigint): api.HrTime {
Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry-core/test/common/time.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ describe('time', () => {
millisecondsToNanoseconds(123.123_456_789),
123_123_457n
);
assert.strictEqual(millisecondsToNanoseconds(1066.969834), 1066969834n);
});
});

Expand Down
Loading