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

Create a report of integrations using deprecated LOA ACR values #10562

Merged
merged 1 commit into from
May 9, 2024

Conversation

vrajmohan
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket: https://gitlab.login.gov/lg-people/lg-people-appdev/protocols/common/-/issues/2

🛠 Summary of changes

Created a report of integrations using deprecated LOA ACR values.

📜 Testing Plan

  • Run the report using aws-vault exec $DEPLOYED_ENVIRONMENT-$LOGIN_IAM_PROFILE -- bundle exec rails runner lib/reporting/protocols_report.rb --date=$SOMEDATE.
  • Confirm the validity of the reported metrics by querying CloudWatch logs from the AWS console

@vrajmohan vrajmohan requested a review from Sgtpluck May 6, 2024 22:22
@vrajmohan vrajmohan force-pushed the loa-requests-report branch from 6dd13b2 to 5306e17 Compare May 8, 2024 20:23
See
https://gitlab.login.gov/lg-people/lg-people-appdev/protocols/common/-/issues/2

**Why**:
-  We would like to like know how many service providers are still using
   the deprecated LOA ACR values.

**How**:
- Using a similar pattern to other reports, query our CloudWatch logs to
  report from the analytics events log.

changelog: Internal, Reporting, Create LOA ACR requests  report
@vrajmohan vrajmohan force-pushed the loa-requests-report branch from 5306e17 to 553084e Compare May 8, 2024 20:58
Copy link
Contributor

@Sgtpluck Sgtpluck left a comment

Choose a reason for hiding this comment

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

a small question but looks good to me

to: time_range.end,
).
map { |slice| slice['issuer'] }.
uniq
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] since you're running dedup in the query, is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed because the same issuer may be present in different time slices; the dedup would only make them unique within a slice.

from: time_range.begin,
to: time_range.end,
).
map { |slice| slice['issuer'] }.
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to fix this in this PR, but i have been having a hard time with the word slice in this context (and other places it's used in maps) especially since we have a different slice var on the instance. we should probably go back and change the references to something that make more sense in the local context, but it's not blocking.

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 inherited the naming from mfa_report.rb. The fetched results from CloudWatch are row slices, ie they could be a full row from the original query (if the time slice and the period coincided) or they could be a row split into time slices. We could use row or row_slice. I don't like row as it hides the fact that it could have been split up. I would prefer row_slice, or leaving it as slice.

Copy link
Contributor

Choose a reason for hiding this comment

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

i understand why slice was used. i still think reusing the variable slice can be confusing at a glance.

@vrajmohan vrajmohan merged commit 4d88740 into main May 9, 2024
2 checks passed
@vrajmohan vrajmohan deleted the loa-requests-report branch May 9, 2024 15:13
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.

2 participants