Take timestamp as an argument in Log::Entry initializer

[Sija]

I want to store re-created Log::Entries from another logging system. In order to do so I need to set Log::Entry#timestamp property, which atm is being set to Time.local on object creation.

[asterite]

I think this is fine, but could you explain a bit the use case?

[Sija]

Example of a Logger -> Log::Backend bridge:

require "logger"

class Logger
  protected getter __log_backend : Log::Backend { LoggerBackendBridge.new }

  private def write(severity, datetime, progname, message)
    level = Log::Severity.parse?(severity.to_s) || Log::Severity::Debug

    __log_backend.write Log::Entry.new(
      source: progname.to_s,
      severity: level,
      message: message.to_s,
      data: Log::Metadata.new,
      exception: nil,
      timestamp: datetime, # <- Impossible
    )
    previous_def
  end
end

[z64]

This is 👍 from me -

In our application stack, we migrated from an in-house logging stack (custom web UI for reviewing telemetry in graphs, statistics, etc.) that was backed by postgres. Starting with 0.34, we moved to an ELK stack, in which we emit logs using the new Log API. We still have that large backlog of log data collected in postgres - being able to create an Entry "resurrected" from these custom postgres rows to emit to our ELK stack is likely the approach we will take, as its a very short script to do so - which this patch would support.

Another small note, I monkeypatch setter timestamp into Entry in our spec suite to test our custom logging formatter's output to set a predetermined timestamp so that formatter output is the same each time. This patch would also remove that, so I can just use the constructor.

[Blacksmoke16]

Does it make sense for the timestamp to not be UTC? I would think UTC would make more sense, especially when trying to consume the logs generated by two applications in two different regions.

[Sija]

Does this needs any further clarifications, or can I get a review so we could get the ball rollin'?

[asterite]

@Sija It's weekend, probably not many people are reviewing code.

[asterite]

Plus the core team has an agenda, and these PRs just take time off of that agenda. So they will be reviewed, but with time.

[Sija]

Plus the core team has an agenda, and these PRs just take time off of that agenda. So they will be reviewed, but with time.

I reckon that, though I'm sure that a 2 line change won't take too much time off the agenda...

[straight-shoota]

@Blacksmoke16 Would you open a new issue for discussing the default value? That's a different topic.

[bcardiff]

I would prefer if the timestamp is moved to a named argument. If we want to add more data to the entry having timestamp there will be inconvenient.

[Sija]

@bcardiff Done. Should be GTG now.

[Sija]

Anything left to do here?