No 'publishTime' field in PubSub messages

[igorclark]

Hi folks, as per the doc, the message objects provided to the subscription.on( 'message' ) listener/callback function don't contain the publishTime field:

// Register a listener for `message` events.
subscription.on('message', function(message) {
  // Called every time a message is received.
  // message.id = ID used to acknowledge its receival.
  // message.data = Contents of the message.
  // message.attributes = Attributes of the message.
});

IIUC PubSub doesn't guarantee message ordering, so having that timestamp in the messages would help with trying to buffer & order at the recipient end - but it's not there. From the comment it seems this was deliberately not included in the node.js client message implementation:

Simplify a message from an API response to have three properties, `id`,
`data` and `attributes`. `data` is always converted to a string.

Is this still desired? Might it make it in? Would a PR to include it be welcome, or is there a strong reason why it's not there?

Thanks!
Igor

Environment details

• OS: Tested on both OSX 10.11.6 and alpine:latest inside Docker container running on OSX
• Node.js version: v7.1.0 and v6.7.0 respectively
• npm version: 3.10.9 and 3.10.3 respectively
• google-cloud-node version: @google-cloud/pubsub@0.6.0 (via npm install --save @google-cloud/pubsub)
• Tested against local emulator and GCP PubSub instance

Steps to reproduce

1. require google-cloud/pubsub
2. create a topic
3. subscribe to the topic
4. publish a message to the topic
5. console.log( JSON.stringify( message ) );
6. message has no 'publishTime' field as per PubSub docs.

{"ackId":"projects/my-pubsub-test/subscriptions/sdfdsf234-1480376751660:262","id":"225","data":{"channel":"sdfdsf234","from":"58e73d48-29ea-4cd0-b46b-085fd1bf89dd","msg":"my message"},"attributes":{}}

I added console.log( msg ); on line 341 of node_modules/@google-cloud/pubsub/src/subscription.js to check the publishTime data is returned by PubSub, and it certainly seems to be:

{ ackId: 'projects/my-pubsub-test/subscriptions/sdfdsf234-1480376751660:262',
  message:
   { data: 'eyJyb29tX25hbWUiOiJzZGZkc2YyMzQiLCJmcm9tIjoiNThlNzNkNDgtMjllYS00Y2QwLWI0NmItMDg1ZmQxYmY4OWRkIiwibXNnIjoibXkgbWVzc2FnZSJ9',
     attributes: {},
     messageId: '225',
     publishTime: { seconds: '1480376756', nanos: 473000000 } } }

(NB when running against the emulator, the nanos field is always 0.)

[stephenplusplus]

The issue was worth the wait :) And a PR would be great! What do you think about naming it 'timestamp' instead of publishTime?

[igorclark]

Thanks @stephenplusplus! It's only a few lines to add that & update the doc comments, so I'll make a PR in a mo.

Personally I prefer timestamp, but the PubSub docs say that it represents The time at which the message was published, populated by the server when it receives the topics.publish call - which seems so carefully named that I'd usually be inclined to leave it alone. What do you think?

[stephenplusplus]

I think start with timestamp for the PR and we can get some more thoughts then. Thanks again for the help!

[igorclark]

Sure thing. However on looking into it I've realised that timestamp implies something different from the { seconds: '1480376756', nanos: 473000000 } object that comes through from the underlying object, and which looks like it's representing the proto definition. So I'll need to have a little think about that. The PubSub API doc says the original format is A timestamp in RFC3339 UTC "Zulu" format, accurate to nanoseconds. Example: "2014-10-02T15:01:23.045123456Z", which makes sense - but do you think it makes most sense to convert it back to that, or to a higher-precision int timestamp, or just to leave it in the object format converted (I assume) from the protobuf?

I also just read the 'Contributing' section in a bit more detail, and it's a fair bit more involved than I thought - not just the testing, but even just clearing the CLA is kind of a big deal, especially for a few lines of code.

On top of that it seems the pubsub client requires node-uuid which raises the npm warning npm WARN deprecated node-uuid@1.4.7: use uuid module instead. Should I just ignore that for now?

So it might take me a little longer than I expected to do this. Will look further into it tomorrow. Thanks for taking the time to respond!

[stephenplusplus]

{ seconds: '1480376756', nanos: 473000000 } object that comes through from the underlying object, and which looks like it's representing the proto definition

Good call, we should convert it to a Date object. Here's a snippet from the Datastore API (https://github.com/GoogleCloudPlatform/google-cloud-node/blob/baf0986e58a667b2cf406c07fd15171857e92afe/packages/datastore/src/entity.js#L201):

var milliseconds = parseInt(value.nanos, 10) / 1e6;
return new Date(parseInt(value.seconds, 10) * 1000 + milliseconds);

I also just read the 'Contributing' section in a bit more detail, and it's a fair bit more involved than I thought

Yeah :\ Let me know what you decide. Even just the issue and idea is helpful enough, so no worries.

On top of that it seems the pubsub client requires node-uuid which raises the npm warning npm WARN deprecated node-uuid@1.4.7: use uuid module instead. Should I just ignore that for now?

Yeah, we can ignore that for now. We will update all of our modules soon which depend on that.

[igorclark]

Thanks @stephenplusplus, that's great.

I've made a commit in my fork (and pasted the diffs in a gist) which does the conversion, tests it, and inserts a couple doc lines in relevant examples. I won't be able to get on to the CLA and system test stuff today; it's literally a few lines though, so if it's in line with what you expect, then if anyone who'd already done the CLA wanted to make a PR with their version of the changes that'd be fine with me :-)

[stephenplusplus]

Thank you very much! I've sent a PR in #1843. Feel free to take a look!

[igorclark]

Looks good to me, thanks @stephenplusplus 👍