Include milliseconds when serializing ActiveSupport::TimeWithZone to JSON

[zorab47]

Problem

Stitches overrides the to_json method on ActiveSupport::TimeWithZone objects but doesn't include sub-second precision even though that is the default in recent versions of Rails.

In many Stitch Fix apps, timestamps with timezone information is being serialized without millisecond precision which impacts messages sent through our messaging infrastructure. Specifically, the recent call-out was for shipment events being sent within the same second, but received out of order without a way of sorting the events (Slack).

[6] pry(main)> Time.zone.now.iso8601
=> "2021-02-11T14:31:53-08:00"
[7] pry(main)> Time.zone.now.iso8601(3)
=> "2021-02-11T14:31:55.113-08:00"

The override of to_json also impacts the encoding of timestamps like created_at and updated_at returned in other contexts (JSON in messages).

Solution

Rails 4.1 introduced a time_precision configuration that defaults to 3, but this invocation of iso8601 doesn't include any sub-second precision.

PR: rails/rails#9128

See ActiveSupport.time_precision or config.active_support.time_precision in: https://guides.rubyonrails.org/configuring.html#configuring-active-support

[zorab47]

What versions of Rails are we supporting with this library? The gem spec denotes any version of Rails:

stitches/stitches.gemspec

Line 21 in 6737805

s.add_runtime_dependency("rails")

[emilyst]

What versions of Rails are we supporting with this library? The gem spec denotes any version of Rails:

stitches/stitches.gemspec

Line 21 in 6737805

s.add_runtime_dependency("rails")

If we're especially concerned about supporting Rails 4.0 or older, we could add a guard to protect against calling this method at runtime if it doesn't exist.

[emilyst]

This seems pretty straightforward and helpful.

[zorab47]

The build matrix goes back to Rails 5.2 and is passing, so I'd assume that's the limit of support. Ruby 2.7 and Rails 5.2

[zorab47]

An alternate implementation could be this, which I'm leaning toward.

def as_json(options = nil)
  if utc?
    super
  else
    utc.as_json(options)
  end
end

[zorab47]

Here's the latest Rails implementation of TimeWithZone#as_json for reference, which we could mimic as well.

    def as_json(options = nil)
      if ActiveSupport::JSON::Encoding.use_standard_json_time_format
        xmlschema(ActiveSupport::JSON::Encoding.time_precision)
      else
        %(#{time.strftime("%Y/%m/%d %H:%M:%S")} #{formatted_offset(false)})
      end
    end

https://github.com/rails/rails/blob/a82df4c5e0e26deed76ddc37ffd95a86455d8265/activesupport/lib/active_support/time_with_zone.rb#L167-L173

[emilyst]

What I'm wondering about is, what was this little patch meant to accomplish? Forcing the timezone or the serialization format?

[emilyst]

Also, is there any reason now that we can't just rely on Rails' behavior?

[brettfishman]

Blocking the merge until @zorab47 and I can connect offline.

[zorab47]

@brettfishman and I connected to discuss this change. I will first test out this functionality in a release candidate before merging it into the main branch.

[soulcutter]

that is the default in recent versions of Rails.

What versions? I'm wondering if this should be represented by logic that switches behavior based on version? But maybe not...

This needs a test.

[zorab47]

@soulcutter recent here means Rails 4.1 and newer; milliseconds were introduced in rails/rails#9128 in 2013.

It isn't explicitly stated in Stitches, but the build matrix only tests against Rails 5.2 and newer right now. That leads me to assume only 5.2 and newer are supported.

[micahhainlinestitchfix]

One potential issue is that our existing guidelines in the eng-wiki forbid this, and there are programmatic checks in some parts of the system at least to forbid this.
I'm not sure all the places that might be impacted, but one is the be_iso8601_utc_encoded check in the test helpers in Stitches.
Any decimal fractional value on the lowest used time precision is valid ISO 8601, but obviously only a subset of processors may support all that.
I seem to recall some errors happening specifically related to subsecond precision being added or not added at some point, so I have little doubt that if we simply started introducing this change into the system without preparing for it that we would have downstream problems related to that.
I would also suggest that if we are doing comparisons by clock time, some events may be disordered based on differing clock times across machines. The only real precision we have at this level is for timestamps on the same machine. That's not an argument against this change, just a caution.
All other things being equal, I would be cautiously supporting of this change, as it does seem to be a reasonable concept, but I wouldn't support it in it's current form. It would need to have all stitches timestamp handling enhanced and coordination time given to teams.

I would suggest if moving forward that it be done in the following steps:

1. Broadly announce the need for the change along with a way to come to alignment on it. Forward The Foundation seems the most likely place to get this in, but it would need to be coordinated with Algo as well.
2. In the first phase of implementation, every team should be mandated to ensure their systems work with subsecond precision, including the ability to store subsecond precision, but they should not generate subsecond timestamps themselves yet. This should include things like the iOS app, which I believe will crash if presented subsecond timestamps (this may or may not have been corrected at some point, or I could be misremembering). It may be that a support window will need to be at least six months long in order to sunset old versions that don't work with these timestamps.
3. Once this deadline has passed, a phase two could start by which all timestamps are mandated to include subsecond precision where available. I would suggest a common and universal format that always includes the same decimal precision, at least as a suggested way to do that.

That sounds like a lot, I deeply feel how painful that would be, but I think that would be the safe way to do this.

[emilyst]

Hi, Micah, I just wanted to acknowledge the care you put into making sure this is the right solution for our problem. I'd also like to respond some specific points you make.

One potential issue is that our existing guidelines in the eng-wiki forbid this, and there are programmatic checks in some parts of the system at least to forbid this.

There is an interesting, strange thing here. Many services already do send millisecond precision for these timestamps. No idea why or what the difference is.

I'm not sure all the places that might be impacted, but one is the be_iso8601_utc_encoded check in the test helpers in Stitches.
Any decimal fractional value on the lowest used time precision is valid ISO 8601, but obviously only a subset of processors may support all that.
I seem to recall some errors happening specifically related to subsecond precision being added or not added at some point, so I have little doubt that if we simply started introducing this change into the system without preparing for it that we would have downstream problems related to that.
I would also suggest that if we are doing comparisons by clock time, some events may be disordered based on differing clock times across machines. The only real precision we have at this level is for timestamps on the same machine. That's not an argument against this change, just a caution.

You are completely correct that times (especially fractions of a second) will often be out of sync across various systems. However, the messages emitted by one system would usually have monotonically increasing timestamps. (This is ignoring an enormous number of other factors, obvs, but those cases are usually negligible.) So it wouldn't be for naught.

In other words, I agree with you, but wanted to add this.

All other things being equal, I would be cautiously supporting of this change, as it does seem to be a reasonable concept, but I wouldn't support it in it's current form. It would need to have all stitches timestamp handling enhanced and coordination time given to teams.

I would suggest if moving forward that it be done in the following steps:

1. Broadly announce the need for the change along with a way to come to alignment on it. Forward The Foundation seems the most likely place to get this in, but it would need to be coordinated with Algo as well.
2. In the first phase of implementation, every team should be mandated to ensure their systems work with subsecond precision, including the ability to store subsecond precision, but they should not generate subsecond timestamps themselves yet. This should include things like the iOS app, which I believe will crash if presented subsecond timestamps (this may or may not have been corrected at some point, or I could be misremembering). It may be that a support window will need to be at least six months long in order to sunset old versions that don't work with these timestamps.
3. Once this deadline has passed, a phase two could start by which all timestamps are mandated to include subsecond precision where available. I would suggest a common and universal format that always includes the same decimal precision, at least as a suggested way to do that.

That sounds like a lot, I deeply feel how painful that would be, but I think that would be the safe way to do this.

Thanks again for carefully articulating this. I agree with what you've stated here.