-
Notifications
You must be signed in to change notification settings - Fork 26
Add SentTimestamp to SQSMetadata #285
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
base: dev
Are you sure you want to change the base?
Add SentTimestamp to SQSMetadata #285
Conversation
/// <summary> | ||
/// The time at which the message was sent to the queue (epoch time in milliseconds). | ||
/// </summary> | ||
public string? SentTimestamp { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the 'long' timestamp a string? Is it stored in the json itself as a quoted string? Even if so, is there any possibility to have non-numerical value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it as a string?
for three main reasons:
- The AWS SQS API itself returns this value as a string within the JSON payload, as documented in here.
- The underlying data structure for all SQS message attributes is a
Dictionary<string, string>
, so it's read as a string by default. - I'm accustomed to converting this into a
long
myself, then intoDateTimeOffset
if needed, when I directly use theMessage
class. It might be best to maintain the same behavior.
However, I agree that converting it to a long
in the SQSMetadata
class is more appropriate, as there's no indication in the documents that it could be anything else. I can update my PR if that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not one to decide, I just stumbled on that since it looks quite suspicious :-)
(and I might have to propagate it correctly in my serializers PR if your goes first, hence the questions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.😄 In that case, let's wait for the team's decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normj any preference on which one makes more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I do agree this is a useful to add.
For the better user experience I think this library should do the conversion to DateTime
from the epoch. It is only a string
in the SDK because the timestamp is being given in the generic message attribute class. One of the goals of this library is to make the experience working with SQS more .NET idiomatic then the generated SQS client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normj, any particular reason for DateTime
instead of DateTimeOffset
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds SentTimestamp support to the SQS metadata handling functionality by exposing the SQS message system attribute for timestamp information.
- Adds
SentTimestamp
property to theSQSMetadata
class - Maps the SentTimestamp attribute in the message metadata handler
- Updates version tracking for the AWS.Messaging package
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/AWS.Messaging/SQSMetadata.cs | Adds SentTimestamp property with documentation |
src/AWS.Messaging/Serialization/Handlers/MessageMetadataHandler.cs | Maps SentTimestamp attribute from SQS message |
.autover/changes/45a6ab22-5bb9-4b65-bfb1-94571b502622.json | Version tracking for the new feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
#283
Adds the SentTimestamp message system attribute to the SQSMetadata class and maps it in the MessageMetadataHandler.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.