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

feat(logger): time zone aware timestamp in Logger #2710

Conversation

arnabrahman
Copy link
Contributor

@arnabrahman arnabrahman commented Jun 29, 2024

Summary

This PR introduces a new feature allowing customers to emit timestamps with a timezone-aware format in the Logger. By default, Logger emits logs with timestamps formatted using the UTC timezone, which is the default behavior in AWS Lambda.

Changes

  • When a timezone other than UTC is set, timestamps will be formatted to include the relevant timezone offset.
    Example: If the emitted timestamp format in UTC is 2016-06-20T12:08:10.000Z and the timezone is set to America/New_York, the timestamp will be formatted as 2016-06-20T08:08:10.000-04:00.

  • The timezone offset is formatted according to the ISO_8601 specification, where the offset can be positive (+) or negative (-) and is followed by HH:MM - see ISO_8601 spec

  • Unit tests have been written for two different timezones to cover both GMT (+) and GMT (-) timezones.

  • While writing the unit tests, I thought if I changed the system timezone by setting the TZ variable, it would work in that timezone. But this is not the case. There are some limitations to jest (Setting process.env.TZ does not affect Dates jestjs/jest#9856, Test cannot change process environment jestjs/jest#9264). I had to mock the getTimezoneOffset function to make it work. There are some workarounds mentioned about custom environment but I couldn't make those work properly.

  • Updated the docs for the new feature, followed the python powertools docs for inspiration.

  • But now comes the most surprising and confusing part. All the tests were running fine locally, I wanted to try it inside a Lambda function, so I deployed one with my solution. However, during testing, I got some unexpected results. The solution works fine when I set the TZ environment variable for different timezones. But when I don't set the TZ environment variable, things just don't work. My implementation was something like this:

    const defaultTimezone = 'UTC';
     if(configuredTimezone && configuredTimezone !== defaultTimezone)
        // Timezone aware implementation
     else
       // UTC timezone format
    

    In cases where there is no TZ set, the if block was getting executed unexpectedly. I spent some time to figure it out what's wrong with my code but I couldn't.

    Then i found that the actual TZ value that AWS Lambda sets is not UTC, it's actually :UTC. I tried with different runtimes like python/node/ruby but got the same result. This is the most confusing part, I don't know if I am missing something in the doc or not.

    After the revelation, I changed the implementation & it worked fine in the lambda.

    const defaultTimezone = 'UTC';
    if (configuredTimezone && !configuredTimezone.includes(defaultTimezone))
      // Timezone aware implementation
    else
      // UTC timezone format
    

Issue number: #1774


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@arnabrahman arnabrahman requested a review from a team June 29, 2024 13:21
@arnabrahman arnabrahman requested a review from a team as a code owner June 29, 2024 13:21
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation logger This item relates to the Logger Utility tests PRs that add or change tests labels Jun 29, 2024
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Jun 29, 2024
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Jun 29, 2024
@dreamorosi
Copy link
Contributor

Hi @arnabrahman, nice to see you again 😃

Thank you so much for opening this PR! I should be able to provide a first review by Monday EOD at the latest.

I want to set aside some time to build the package and test it in a real Lambda function to see how it behaves.

@dreamorosi dreamorosi linked an issue Jun 30, 2024 that may be closed by this pull request
2 tasks
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @arnabrahman - thank you for the excellent PR and also for the investigation in the TZ value on Lambda.

I have checked internally and I have been told that the :UTC value (including the :) is intended and that was set like this to be compliant with the posix standard.

With that said, I agree that the docs are confusing and I'll be suggesting an update to that page to clarify the actual value. If you want to speed up the process, consider doing the same via the "Feedback" button in the doc page (Thank you!).

Regarding the PR, I have tested a build of this branch on Lambda and was able to confirm that it works as intended.

I have left a few suggestions for changes in the docs, mainly to format the new paragraph and rework the code snippets. This latter one especially is the biggest change from your PR.

In real world usage we don't recommend to instantiate a Logger instance inside the Lambda handler, this is so that the logger can be reused across invocations. However I understand what you were going for, so I have instead changed the sample to create a child logger after changing the TZ value.

Looking forward to merge this once the feedbacks are addressed! Thanks again for the great work!

@dreamorosi dreamorosi self-requested a review July 1, 2024 11:18
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above

@arnabrahman
Copy link
Contributor Author

arnabrahman commented Jul 2, 2024

I see the doc has been updated.

@arnabrahman arnabrahman requested a review from dreamorosi July 2, 2024 13:39
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing the changes and the quick turnaround!

Yes, I noticed as well, well spotted!

I'm marking the PR as approved, however I need to investigate why one of the automation checks is not running - please allow me some time, I'll merge this as soon as it's fixed

@dreamorosi dreamorosi merged commit 9fe4c75 into aws-powertools:main Jul 3, 2024
9 checks passed
@arnabrahman arnabrahman deleted the 1774-logger-timezone-aware-timestamp branch July 5, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature PRs that introduce new features or minor changes logger This item relates to the Logger Utility size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: time zone aware timestamp in Logger
2 participants