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.

Sign up for GitHub

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

Jump to bottom

log/slog: slog.Time, doesn't handle dates after year 2262 #65902

Closed
mvrhov opened this issue Feb 23, 2024 · 11 comments
Closed

log/slog: slog.Time, doesn't handle dates after year 2262 #65902

mvrhov opened this issue Feb 23, 2024 · 11 comments
Assignees
@jba
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@mvrhov
Copy link

mvrhov commented Feb 23, 2024 •
edited
Loading

Go version

1.21, 1.22

Output of go env in your module/workspace:

/

What did you do?

package main

import (
	"log/slog"
	"os"
	"time"
)

func main() {
	logger := slog.New(slog.NewJSONHandler(os.Stdout, nil))
	logger.Info("time test", slog.Time("expires_at", time.Date(2300, 1, 1, 0, 0, 0, 0, time.UTC)))
}

This is due to converting time.Time to nanosecond in slog.Time

What did you see happen?

{"time":"2009-11-10T23:00:00Z","level":"INFO","msg":"time test","expires_at":"1715-06-13T00:25:26.290448384Z"}

What did you expect to see?

I expected to see date in future e.g 2300-01-01 00:00:00.0Z in this case
@mvrhov mvrhov changed the title log/slog: slog.Time, doesn't handle dates after 2262 log/slog: slog.Time, doesn't handle dates after year 2262 Feb 23, 2024
@panjf2000
Copy link
Member

panjf2000 commented Feb 23, 2024 •
edited
Loading

Please check out Time.UnixNano:

The result is undefined if the Unix time in nanoseconds cannot be represented by an int64 (a date before the year 1678 or after 2262).

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Feb 23, 2024
@mvrhov
Copy link
Author

mvrhov commented Feb 23, 2024

Can I have an explanation on why this was closed. This is a legit bug report. I know that this is because of the use of time.UnixNano, and I have also written this into the original ticket. This is due to converting time.Time to nanosecond in slog.Time
IMHO this should be reopened and the slog must log any valid date time.Time supports. Unless you are saying that's OK, to have weird dates in the log, when you use dates after a certain one.

@seankhliao
Copy link
Member

seankhliao commented Feb 23, 2024 •
edited
Loading

sorry, it was a misclick

cc @jba

@seankhliao seankhliao reopened this Feb 23, 2024
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 23, 2024
@bcmills
Copy link
Contributor

bcmills commented Feb 23, 2024

slog must log any valid date time.Time supports.

Unfortunately “any valid date time.Time supports” is currently ill-defined. See:

@seankhliao
Copy link
Member

also #63844

@mvrhov
Copy link
Author

mvrhov commented Feb 23, 2024

Unfortunately “any valid date time.Time supports” is currently ill-defined. See:

int16 for a year would probably suffice, like in the ticket mentioned by @seankhliao :), but now I'm off-topic.

@jba
Copy link
Contributor

jba commented Feb 23, 2024

I'm happy to leave this open, but as pointed out here, it is just another consequence of what #63844 is trying to fix. So I don't think there is any action to take now.

@seankhliao
Copy link
Member

maybe we can just document the more limited range here?

@panjf2000
Copy link
Member

maybe we can just document the more limited range here?

That seems to be the only thing we can do so far.

@panjf2000 panjf2000 added Documentation Issues describing a change to documentation. help wanted labels Feb 24, 2024
@panjf2000 panjf2000 added this to the Backlog milestone Feb 24, 2024
@rsc
Copy link
Contributor

rsc commented May 16, 2024

This issue is about slog happening to have picked a very tiny time format compared to time.Time in its zeal for avoiding allocations. This has nothing to do with #63844, which is about optimizing the time package. It's true that the issue suggests restricting the time range, but to something like 60,000 years, not the mere 500 that slog allows.

I think we should probably fix this by making the time constructor store an actual time.Time into v.any when the value is out of range for UnixNano. That would still be allocation-free for all the times anyone would ever see, but it also avoids slog introducing time range restrictions that do not exist in package time.

/cc @jba

@jba jba self-assigned this May 16, 2024
@jba jba added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation Issues describing a change to documentation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. help wanted labels May 16, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585519 mentions this issue: log/slog: handle times with undefined UnixNanos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jba jba

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @mvrhov @bcmills @panjf2000 @gopherbot @seankhliao @jba