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

Switch SpanEvent times to use time.Time #1101

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Sep 12, 2024

Use time.Time, a timestamp, to identify the start and end time of the SpanEvent instead of representing them as nanoseconds since boot time in a built-in int64.

This is motivate by the need to support things that report timestamps for start/end times directly. Things like the custom SDK (see #1045).

To support the conversion for probes, the utils.BootOffsetToTime function is added. This converts between the measured nanoseconds since boot-time that an eBPF program measures, and a timestamp.

The inverse function, utils.TimeToBootOffset, is also added to facilitate testing.

@MrAlias MrAlias requested a review from a team September 12, 2024 20:21
@MrAlias MrAlias marked this pull request as draft September 12, 2024 20:21
Use `time.Time`, a timestamp, to identify the start and end time of the
`SpanEvent` instead of representing them as nanoseconds since boot time
in a built-in `int64`.

This is motivate by the need to support things that report timestamps
for start/end times directly. Things like the custom SDK (see open-telemetry#1045).

To support the conversion for probes, the `utils.BootOffsetToTime`
function is added. This converts between the measured nanoseconds since
boot-time that an eBPF program measures, and a timestamp.
@MrAlias MrAlias marked this pull request as ready for review September 12, 2024 21:11
Copy link
Contributor

@RonFed RonFed left a comment

Choose a reason for hiding this comment

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

LGTM, if there is a way to call this conversion in the generic probe so we can do it in one place it is better but I don't have a strong opinion about it.

internal/pkg/instrumentation/utils/kernel.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 15, 2024

if there is a way to call this conversion in the generic probe so we can do it in one place it is better

All of the convertEvent functions are converting a probe specific event type. I don't think there is a more generic way to make this conversion than with the provided functions.

@RonFed
Copy link
Contributor

RonFed commented Sep 15, 2024

if there is a way to call this conversion in the generic probe so we can do it in one place it is better

All of the convertEvent functions are converting a probe specific event type. I don't think there is a more generic way to make this conversion than with the provided functions.

My point was that before this change the conversion was done in one place (the controller) and now it is duplicated across all the probes. As I said I don't have a strong opinion about this.

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 16, 2024

Understood. Keep in mind, with the addition of the SDK not all events are going to report time as an offset from boot-time. This change is made to support that addition.

This also means there is not going to be a generic way to handle all event times given they would be timestamps or offsets from boot-time.

I see it as preferable to unify on using timestamps, as proposed here, and convert the boot-time offsets so when we allow users to send us data1 they can rely on standard Go types and time measurements.

Footnotes

  1. Custom Probes #1105

@MrAlias MrAlias merged commit 567526e into open-telemetry:main Sep 18, 2024
24 checks passed
@MrAlias MrAlias deleted the event-timestamps branch September 18, 2024 21:14
@MrAlias MrAlias added this to the v0.15.0-alpha milestone Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants