Skip to content

Fix CloudwatchClient error when run with a nil timezone#9441

Merged
zachmargolis merged 2 commits intomainfrom
margolis-fix-cloudwatch-error-timezone
Oct 24, 2023
Merged

Fix CloudwatchClient error when run with a nil timezone#9441
zachmargolis merged 2 commits intomainfrom
margolis-fix-cloudwatch-error-timezone

Conversation

@zachmargolis
Copy link
Contributor

when we run the ./bin/query-cloudwatch script, we're running outside of Rails, so Time.zone is nil, so we get errors like this. Switching to Time.at lets us work around this

Example:

identity-idp/lib/reporting/cloudwatch_client.rb:169:in `rescue in fetch_one': undefined method `at' for nil:NilClass (NoMethodError)

        log(:warn, "query end_time=#{end_time} (#{Time.zone.at(end_time)}) is before Cloudwatch Insights availability, skipping")
                                                           ^^^
  from identity-idp/lib/reporting/cloudwatch_client.rb:155:in `fetch_one'
  from identity-idp/lib/reporting/cloudwatch_client.rb:88:in `block (2 levels) in fetch'
.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/aws-sdk-core-3.179.0/lib/seahorse/client/plugins/raise_response_errors.rb:17:in `call': End time should not be before the service was generally available (Service: AWSLogs; Status Code: 400; Error Code: InvalidParameterException; Request ID: c53d3718-df37-436c-a510-02675dd2141c; Proxy: null) (Aws::CloudWatchLogs::Errors::InvalidParameterException)

changelog: Internal, Scripts, Fix error when querying cloudwatch with no timezone
@zachmargolis zachmargolis requested a review from a team October 24, 2023 17:54
Copy link
Contributor

@ThatSpaceGuy ThatSpaceGuy left a comment

Choose a reason for hiding this comment

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

Looks good!

# rubocop:enable Layout/LineLength
# rubocop:disable Layout/LineLength, Rails/TimeZone
log(:warn, "query end_time=#{end_time} (#{Time.at(end_time)}) is before Cloudwatch Insights availability, skipping")
# rubocop:enable Layout/LineLength, Rails/TimeZone
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, Rubocop, for complaining that we're not reintroducing the bug. 😆

gif of Robery Downey Jr. sighing and facepalming

}

# override Zonebie
allow(Time).to receive(:zone).and_return(nil)
Copy link
Contributor

@n1zyy n1zyy Oct 24, 2023

Choose a reason for hiding this comment

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

Does this imply zonebie is calling Time.zone itself? No issue with it review-wise, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried setting Time.zone = nil in a before filter, in the spec itself, etc, and they all got overridden, so I had to stub to get the spec to actually run with a nil timezone

@zachmargolis zachmargolis merged commit a5d4be6 into main Oct 24, 2023
@zachmargolis zachmargolis deleted the margolis-fix-cloudwatch-error-timezone branch October 24, 2023 18:33
@mdiarra3 mdiarra3 mentioned this pull request Oct 26, 2023
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

Successfully merging this pull request may close these issues.

3 participants