Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EdgeHub fails to parse valid D2C creation time UTC string #7294

Open
stefanb2 opened this issue May 23, 2024 · 6 comments
Open

EdgeHub fails to parse valid D2C creation time UTC string #7294

stefanb2 opened this issue May 23, 2024 · 6 comments
Assignees

Comments

@stefanb2
Copy link

Expected Behavior

I follow the System Properties of D2C IoT messages table and add a creation time property to event messages. Those messages will be routed

  • from my Azure IoT Edge module (FROM /message/modules/<MODULE>/outputs/<OUTPUT>)
  • via edgeHub
  • to the Azure IoT Hub (INTO $upstream)

Message creation & sending:

// e.g. "2024-05-23T07:50:52.598Z"
const now = new Date().toISOString();
const message = new Message(JSON.stringify(data));

message.properties.add('iothub-creation-time-utc', now);

client.sendOutputEvent(output, message);

This works fine when module uses MQTT protocol to connect to edgeHub:

import { Mqtt } from 'azure-iot-device-mqtt'
import { ModuleClient } from 'azure-iot-device'

client = await ModuleClient.fromEnvironment(Mqtt)

Current Behavior

When I switch the module from MQTT to AMQP protocol (NOTE: no other changes to the code!):

import { Amqp } from 'azure-iot-device-amqp'
import { ModuleClient } from 'azure-iot-device'

client = await ModuleClient.fromEnvironment(Amqp)

then messages are no longer routed from the Azure IoT Edge device to the Azure IoT Hub.

Instead you can now see an exception in edgeHub log, f.ex.

System.FormatException: String '2024-05-23T07:50:52.598Z' was not recognized as a valid DateTime.

The traceback points to this line in the edgeHub source code in DeviceClientMessageConverter.cs:

if (inputMessage.SystemProperties.TryGetNonEmptyValue(SystemProperties.CreationTime, out string creationTime))
{
  message.CreationTimeUtc = DateTime.ParseExact(creationTime, "o", CultureInfo.InvariantCulture);
}

The "o" format in the above line only seems to accept YYYY-MM-DDTHH:MM:SS.1234567Z as valid. I confirmed this by applying the following workaround to my code:

message.properties.add('iothub-creation-time-utc', now.replace(/Z$/, '0000Z'));

With the workaround in place the errors disappeared from the log and messages were routed again from the Azure IoT Edge device to the Azure IoT Hub.

Steps to Reproduce

  1. create an Azure IoT Edge device
  2. create a custom module with AMQP as protocol
  3. see the code snippets in the above description for the message creation
  4. deploy custom module to Azure IoT Edge device
  5. monitor events from the Azure IoT Edge device on the Azure IoT Hub

Context (Environment)

Device Information

  • Host OS: Debian GNU/Linux 12 (bookworm) (tailored build of Raspberry Pi OS)
  • Architecture: arm64
  • Container OS: Linux containers

Runtime Versions

  • aziot-edged: 1.5.0
  • Edge Agent: 1.5
  • Edge Hub: 1.5
  • Docker/Moby: 26.1.2

Additional Information

What is the intention behind the strict DateTime.ParseExact(..., "o", ...) ("The round-trip ("O", "o") format specifier" according to .NET documentation)? It should accept any valid UTC ISO-8601 string.

@arsing arsing assigned yophilav and unassigned arsing Oct 16, 2024
@damonbarry
Copy link
Member

@stefanb2 Thanks for reporting this. It does appear to be a bug in Edge Hub: I believe the example in the docs would also cause the exception you reported because it doesn't include a fraction value before the "Z".

I'm a little reluctant to make any changes here because this code has been in place since the beginning of the project. I worry that any changes here might break someone else who relied on the existing behavior. But perhaps it would be good to investigate whether "downgrading" the parsing call from DateTime.ParseExact to DateTime.Parse could correct this problem without breaking anything.

@ryanwinter
Copy link

Closing this as "won't fix" as there is a workaround and changing the behaviour may introduce unexpected consequences.

Please reopen if you are unable to work around this issue.

@stefanb2
Copy link
Author

stefanb2 commented Nov 1, 2024

Closing this as "won't fix" as there is a workaround and changing the behaviour may introduce unexpected consequences.

This solution is not acceptable.

Pointing to a workaround is invalid, because this broken behavior is not documented.

Please reopen if you are unable to work around this issue.

That option is disabled: "You do not have permission to reopen this issue".

@ryanwinter
Copy link

Reopening issue, sorry I didnt mean to dismiss this issue so quickly, I think I misunderstood the documentation explicitly not aligning with the functionality.

Talking to @damonbarry, perhaps the codepath that @stefanb2 pointed, we could a catch the exception from ParseExact, and then try again with a Parse. This might reduce a chance of a regression?

@ryanwinter ryanwinter reopened this Nov 1, 2024
@jlian
Copy link
Member

jlian commented Nov 12, 2024

We are investigating more.

In the meantime, @PatAltimore and @bishal41 can you help with correcting the doc issue @damonbarry mentioned? Also can we document this as a known issue?

@jlian
Copy link
Member

jlian commented Nov 21, 2024

@ancaantochi and I talked about this. Seems like replacing the call from DateTime.ParseExact to DateTime.Parse might be a breaking change or at least risky for existing clients FYI @damonbarry

@ancaantochi said that's it's probably worth looking into why it doesn't work for AMQP even though the call for MQTT is the same. Might have something to do with the data format handling for AMQP, like maybe it's handled for MQTT but not AMQP. @yophilav would you be able help look into this a bit further?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants