time() printing a string to terminal is fine for some use cases but it's more useful in others to be able to printf() a timestamp.
Making time() return a string would be API breaking and also cause issues with strncmp (because we cannot resolve the timestamp string in kernel) so we probably don't wanna change that.
Perhaps we can make a epoch builtin that returns seconds since epoch? I think seconds as granularity should be enough b/c high precision timing could just use nsecs instead.
epoch builtin to return seconds since epoch.
We could insert the epoch time at system boot in a map during prog load and then use bpf_ktime_get_ns() helper to compute seconds since epoch.
Making timestamp() return a string
Might be useful to have strftime() as a builtin, which accepts a format string, and returns a string. That way we also stick to standard conventions around time formatting.
Might be useful to have
strftime()as a builtin, which accepts a format string, and returns a string. That way we also stick to standard conventions around time formatting.
I don't think we can implement that as we cannot do the string formatting in bpf, so we cannot return an actual string.
I think it should be to re-use time for this without breaking the current behaviour. That way we can introduce the new one and slowly phase out the old one in the next few releases.
That would mean we support both:
i:s:1 { time("xxx"); }
and
i:s:1 { $ts = nsecs + 300000; printf("%s\n", time("xxx", $ts); }
To do this we would have to add a new type, Type::timestamp which just stores the value of nsecs and detect whether time() is used "stand alone" or in assignment/printf.
I think we should make it a "final" type that doesn't allow modification, e.g. time(...) + 300 should be invalid.
This assuming that we can somewhat reliably convert nsecs into a real timestamp. Not sure how accurately we can get the actual time the system booted. But it won't be any less accurate than time() currently is, which gets the timestamp on which the event gets printed by userspace.
To do this we would have to add a new type, Type::timestamp which just stores the value of nsecs and detect whether time() is used "stand alone" or in assignment/printf.
I think we should make it a "final" type that doesn't allow modification, e.g. time(...) + 300 should be invalid.
I thought about it a bit and I think your approach is better. My only concern is that we're adding onto our growing list of primitive types. But I think this solution is better for the user.
This assuming that we can somewhat reliably convert nsecs into a real timestamp. Not sure how accurately we can get the actual time the system booted. But it won't be any less accurate than time() currently is, which gets the timestamp on which the event gets printed by userspace.
I think we can probably use /proc/stat's btime to get second level precision. We can make a note about it in the docs how it's second-level.
There may be some crisis where we do deprecate some syntax, but this time() thing isn't it. I'd just allow these both to work forever:
i:s:1 { time("xxx"); }
i:s:1 { $ts = nsecs + 300000; printf("%s\n", time("xxx", $ts); }
or call the second one strftime().
Let's go with strftime(). It might be confusing to change the behavior of time() to depend on how it was called.
This assuming that we can somewhat reliably convert nsecs into a real timestamp. Not sure how accurately we can get the actual time the system booted. But it won't be any less accurate than time() currently is, which gets the timestamp on which the event gets printed by userspace.
@fbs I'm afraid we cannot count on nsecs to get current time since it's implemented using bpf_ktime_get_ns, which does not include suspended time according to bpf-helpers(7). @javierhonduco reminded me that bpf_ktime_get_boot_ns could be useful here, which was introduced to address the problem according to this.
Unless we can assume that systems running bpftrace would never suspend, we need to change our target. I wouldn't propose we change nsecs to rely on bpf_ktime_get_boot_ns because it seems that nsecs never promised to serve this purpose; I assume it (only) serves to compute elapsed time. What I would suggest is that, we only use strftime to print current time, instead of printing a timestamp provided by users (since they don't have the right tool to get that). We may extend its functionality when we can provide the tool. That is to say we only allow something like:
i:s:1 { printf("%s\n", strftime("%H:%M:%S")); }
i:s:1 { printf("%s\n", strftime("%D")); }
i:s:1 { printf("%s\n", strftime()); } // default format
I'm still quite new to bpftrace. So excuse me if I'm wrong about it.
What I would suggest is that, we only use strftime to print current time, instead of printing a timestamp provided by users (since they don't have the right tool to get that).
I don't believe there's a fundamental limitation here. I think we can make nsecs use bpf_ktime_get_boot_ns if it's available. This new nsecs should be backwards compatible and strictly more accurate.
I also have some serious doubts anyone was relying on the missing suspension time (especially I've never heard of people suspending production machines, and production machines are what I imagine our target users is using bpftrace on).
I think we can go ahead with "using" the current nsecs for now and have a separate discussion for "upgraded" nsecs.
Makes sense to me!
Most helpful comment
I don't believe there's a fundamental limitation here. I think we can make
nsecsusebpf_ktime_get_boot_nsif it's available. This newnsecsshould be backwards compatible and strictly more accurate.I also have some serious doubts anyone was relying on the missing suspension time (especially I've never heard of people suspending production machines, and production machines are what I imagine our target users is using bpftrace on).
I think we can go ahead with "using" the current
nsecsfor now and have a separate discussion for "upgraded"nsecs.