drogue-iot / drogue-cloud

Cloud Native IoT
https://drogue.io
Apache License 2.0
114 stars 30 forks source link

[mqtt] Allow timestamp in MQTT 3.1 publisher #221

Closed JulianFeinauer closed 2 years ago

JulianFeinauer commented 2 years ago

If a sender wants to submit a custom timestamp for a message this cannot be passed as the timestamp of the CloudEvent that will be generated upstream.

So my initial simple idea was to allow people to either specify only the payload for the CloudEvent (i.e. the current behavior) or alternatively a CloudEvent / Cloud Event Stub e.g. if the following conditions are met for the message

In this situation we could interpret the message as a CloudEvent stub and in the MQTT Endpoint just add the not-set parts of the CloudEvent properties.

This would make it easy and elegant to add informations like a timestamp.

Example of case 1 (current behavior):

{"temp":42}

Example of case 2 (to set the timestamp):

{
"time" : "2020-04-05T17:31:00Z",
"data": {"temp":42}
}

Both examples would lead to a CloudEvent upstream with exactly the same payload. Only the timestamp property of the CloudEvent would be different.

JulianFeinauer commented 2 years ago

@ctron I would also be happy to try to get my hands onto this one and help a bit but I didnt find the right starting point yesterday so I might need some guideance :)

ctron commented 2 years ago

I think there are two view on this one, and IIRC, we did discuss this in the past. However, I think it is a valid use case, so I think we should discuss it once more.

@garyedwards if you have a bit of spare time, I would appreciate your feedback on this one too

Currently the time attribute is considered to be the timestamp when the cloud event was generated. Any other timestamp is considered application specific, and belongs into the payload: https://github.com/drogue-iot/rfcs/blob/main/active/0003-cloud-events-mapping.md#core-attributes

On the other hand, we also allow to override other attributes (content type, subject). So why not this one?

Then again, some of the cloud event attributes have an impact on the infrastructure (partionkey) while others need to be trustworthy by the consumers (device id, application id). So we cannot simply accept an cloud event and just forward them.

Also, this would actually have an impact on the payload format (at least for MQTT 3.1.1). Currently our topic structure doesn't support an alternative topic path (like ce/<channel>/<device> is a cloud event, while raw/<channel>/<device> is a raw message.

However, we also discussed that it might make sense to have such an alternative, and that changing the topic structure might make sense. The current alternative would be to create a new endpoint for each variant (times three for the different transport configurations). Which might be a bit too much.

So technically that should be easy. And definitely a good exercise. The question is only, what is the "right" way to deal with this problem.

JulianFeinauer commented 2 years ago

@ctron thanks for the response and I agree with what you write. I did not look to deep into the specification and if the "time" is the "generation time" than my proposal doesnt make sense at all.

And also I agree with you that only certain keys of the CloudEvent should be allowed to be overwritten (time, content type probably such things) but others should not be allowed (like partiion things et al).

I then still see two options here. Either allow to add additional properties to the cloud events OR just use the time in the payload. Technically there is not much difference its rather a question of taste or philosophy, I guess (at least in MQTT3. In the other transports headers may be handled differently).

garyedwards commented 2 years ago

Hi Both,

From my perspective I think it is cleaner to have the CloudEvent be populated by the Drogue IoT alone using values it has generated or values from the protocol endpoints which would otherwise be lost such as sender and subject. I think the current ingress time behaviour is correct.

The information that arrives in the payload feels like it should remain their until is is consumed by a downstream application including any possible timestamps that may be included. Otherwise Drogue could start becoming too application specific.

I guess the failing in my argument is that the HTTP endpoint could accept any envelope values and the other endpoints may have headers that are not currently covered? Some examples from the supported protocols would be good, can MQTT send a timestamp in the header?

I have found the current attributes are enough to work with for our applications and both CloudEvent ingress time and payload timestamp (as per the payload schema we use) are commonly used without issue.

ctron commented 2 years ago

So to me this sounds like it makes sense to keep the current behavior of not handling the time, but leaving this to the payload and the application of top of Drogue Cloud.

Maybe you can give a :+1: or :-1: if you agree/disagree.

ctron commented 2 years ago

@JulianFeinauer are you ok closing this?