eventhub: application properties is not sent

[davidt99]

On version 1.3.2, if I set an application properties key, the properties are not sent:

event_data = EventData(json.dumps({'test': 1}))
event_data.application_properties['foo'] = 'bar'
sender.send(event_data)

The thing is, if I initialize the application properties as a new dictionary, it works:

event_data = EventData(json.dumps({'test': 1}))
event_data.application_properties = {'foo: 'bar'}
sender.send(event_data)

The problem lies here:
https://github.com/Azure/azure-event-hubs-python/blob/master/azure/eventhub/common.py#L92 where this dict is not propagated to the Message object created in line #105
That repo is no longer maintained but I'm not interested in using a SDK which is still in beta status.

[kaerm]

@davidt99 thanks again :) - routing it the right way. //cc: @yunhaoling

[mayurid]

@davidt99 : Thanks for bringing this to out attention. Looping in @KieranBrantnerMagee who will help you with this issue.
On a side note I want to ensure you that EventHubs 1.3.2 is not unmaintained and we as a team are supporting it. The code has moved from the location https://github.com/Azure/azure-event-hubs-python but a new location at https://github.com/Azure/azure-sdk-for-python/tree/eventhub_track1/sdk/eventhub/azure-eventhubs but unfortunately the readme was pointing to the code for the preview SDK. We have since updating this and you should have the right redirect.

[davidt99]

Thanks @mayurid!
I originally wanted to open a PR for this, I just didn't know where the code was.

[KieranBrantnerMagee]

No worries @davidt99, as we prepare for our upcoming releases there was some flux to normalize our repository/lib structure so things moved around, and the way the locations transitioned isn't always immediately obvious.

So as you correctly observed the underlying fix for this was rather straightforward. However, it turns out that wasn't the end of the rabbit hole. The underlying uamqp library version needs to be pulled forward to support application_properties being threaded through properly. However bringing that version forward revealed further fixes which will need to be applied within uamqp to unblock the us here. Luckily @yunhaoling had already come across this while handling other issues so we'll be combining forces to address this.

Will loop back asap as we learn more; thanks for your patience and again for reporting this.

[davidt99]

@KieranBrantnerMagee, thanks for the update. Can you elaborate on the issue you faced? I was able to send and read application properties.

[KieranBrantnerMagee]

Of course.

When I went through to validate this scenario after applying the fix, I noticed there were other tests that evaluated app_properties, e.g. test_receive_batch_with_app_prop_sync

However, you'll notice that they are currently being skipped due to needing a bumped UAMQP version. Given that the fix to address your bits will need to be applied to the batching scenario as well for completeness/consistency, I had been hoping to reenable this to validate that I'm not introducing other issues, but this requires the aforementioned update lest it fails with errors expecting bytes vs eventData.

Given that Adam had already been looking into this, and that the bumped version is what will be used in our vnext APIS (additionally containing fixes to correct proper timeout threading) I erred on the side of applying the fix broadly and validating it in all flows in one go, to your point this may have been overkill for addressing just you and if there are any major blockers/issues with getting the full fix out I would fall back to the limited change to unblock you.

Let me know if that braindump/wall of text was unclear and I can try to clarify. You should also expect a PR to show up in this thread shortly which will contain both the fix for what you pointed out, and the underlying UAMQP version bump and fix. Feel free to eyeball that as an even more literal explanation of "what went wrong."

[davidt99]

@KieranBrantnerMagee Thanks for the clarification!

[annatisch]

@KieranBrantnerMagee - are you able to confirm whether this issue is present in track 2?

[KieranBrantnerMagee]

Validated that the issue is not present in T2. (To be precise in the presence of a double negative: T2 is good :) )

INFO:root:Got receiver event: {'body': 'batchId: 99, event: 69', 'application_properties': "{b'a': b'42'}", 'sequence_number': '969', 'offset': '73608', 'enqueued_time': '2019-11-27 17:50:31.215000'} With application properties: {b'a': b'42'}

[KieranBrantnerMagee]

Closing this issue as the changes should have made it into 1.3.3.

Please don't hesitate to reopen and/or let us know if any further issues help or clarification is needed; Thanks to everyone involved!