Pubsub message timestamp broken

[ptone]

The message timestamp for pubsub is fully broken - it is looking for the timestamp in a message attribute, but per the docs, it is a primary field of the message resource.

https://github.com/GoogleCloudPlatform/gcloud-python/blob/master/gcloud/pubsub/message.py#L51

the docs - clearly reference in source comments point to where the timestamp actually is:
https://cloud.google.com/pubsub/reference/rest/v1/PubsubMessage

In API explorer, here is the returned value:

cache-control:  private
content-encoding:  gzip
content-length:  322
content-type:  application/json; charset=UTF-8
date:  Thu, 17 Mar 2016 01:20:32 GMT
server:  ESF
vary:  Origin, X-Origin, Referer

{
 "receivedMessages": [
  {
   "ackId": "Pn4qKUVBXkASTCEcRElTK0MLKlgRTgQhIT4wPkVTRFAGFixdRkhRNxkIaFEOT14jPzUgKEUXC1MTUVx2BFgQajNcdQdRDRh3fml3aFhCAwBNWX5VWzM9SmJddgRYChl1eGB3aVIbASrfoMQWZhk9XBJLLA",
   "message": {
    "data": "dGhpcyBpcyB0aGUgbWVzc2FnZV9wYXlsb2FkMg==",
    "messageId": "13397075123880",
    "publishTime": "2016-03-17T01:13:41.166Z"
   }
  }
 ]
}

[dhermes]

Thanks @ptone, you are our best bug-filer! Do you have a stacktrace / code snippet to trigger the error?

[tseaver]

FWIW, the Topic.publish wrapper actually adds a timestamp into the message attributes, IFF timestamp_messages is set to True on the topic: that is the value which Message.timestamp expects to consume. The need for such a field was discussed in #797, which quoted the pubsub docs as saying:

Although Pub/Sub usually delivers messages in order of publication, this is not guaranteed; it is possible for subscriptions to receive messages out of order. For this reason, we suggest that you include sequence information in the message payload or attribute so that subscribers that need in-order messaging can implement logic to do so.

Support for generating the attiribute was added to Topic in #809, and the property which parses it out was added to Message in #815.

If I had been aware of the PubsubMessage.publishTime field, I would have just recommended using it, rather than adding an extra attribute.

[dhermes]

@tseaver Does this mean a change is on the way?

[ptone]

I can appreciate the good will of trying to capture a best practice into the class definition. But I think these sort of sweetners should be called out explicitly in general, and in this case there is an unfortunate semantic conflict with the resource originated publishTime

I think there are going to be times you need to preserve strict order from the publisher environment, but I would have rather named the convention for the attribute something like "user_timestamp" - there is nothing about this provided by the service. If you have well coordinated clocks among a set of publishers, the user_timestamp will be more reliable than publishTime - which is going to be an internal service-side time, and is really best effort based on arrival time. It is poorly named, as it is really internalPublishTime between one part of PubSub and another.

I don't have any particularly strong opinion about how to "fix" the semantic conflict here - but I do hope we can see the resource property exposed in the Message class.

[tseaver]

Does this mean a change is on the way?

I'm not sure. The simplest change would be to expose a read-only Message.service_timestamp property. We would then need to be careful to document the differences between the two. Renaming the current Message.timestamp would break any existing consumers: I'm not sure how free we feel to make such changes.

[ptone]

Do you have anything roughly approximating this:
https://docs.djangoproject.com/en/1.9/internals/release-process/#internal-release-deprecation-policy

I would rename .timestamp to .user_timestamp, keep the old property wrapped in a deprecation warning, and change the docs for anyone coming in for the first time.

Then you can make clear in the docs the difference between the two timestamps

[dhermes]

While we are 0.x.y we don't really have issues deprecating features.

[tseaver]

@dhermes, @jgeewax I have a branch ready which adds the server_timestamp property, based on the value set in the resource server-side. Any opinion on whether to rename the existing timestamp property?

[dhermes]

I think we should just remove any manually added timestamp since the API supports it. Though @ptone seems to imply the backend's timestamps might not be reliable?