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

[DEPR-2U]: Remove md4 caching and related ENABLE_BLAKE2B_HASHING feature flag #872

Open
8 of 9 tasks
robrap opened this issue Dec 12, 2024 · 5 comments
Open
8 of 9 tasks
Assignees

Comments

@robrap
Copy link
Contributor

robrap commented Dec 12, 2024

We need to enable this feature flag in advance of its removal, detailed in the following DEPR:

Tim did mention possibly adding telemetry to better understand the impact before enabling in this comment on the PR where the toggle had been added:

Looking to complete this by April 2025.

Acceptance Criteria:

Implementation Tasks:

@robrap robrap added this to Arch-BOM Dec 12, 2024
@robrap robrap moved this to Ready For Development in Arch-BOM Dec 16, 2024
@robrap
Copy link
Contributor Author

robrap commented Dec 16, 2024

First pass at instrumentation:

  • increment('safe_key_called') # count of all calls
  • increment('safe_key_with_hash') # count of all calls taking the hashing branch

I can't remember if we'll actually be able to sum this information in DD? You might need to create an integer-based facet based on the span tag.

Based on these findings, we may want logging with the actual keys, or we may feel it is safe enough if the with-hash percentage is low enough.

@timmc-edx timmc-edx moved this from Ready For Development to In Progress in Arch-BOM Dec 16, 2024
@timmc-edx timmc-edx self-assigned this Dec 16, 2024
@timmc-edx
Copy link
Member

@pdpinch
Copy link

pdpinch commented Dec 23, 2024

FYI, the PR to remove the flag has been opened at openedx/edx-platform#36054

We won't merge it until y'all have resolved & closed this issue.

@timmc-edx
Copy link
Member

Thanks! We're actually ready for this to proceed -- the remainder of this ticket is just communication and post-removal cleanup.

@pdpinch
Copy link

pdpinch commented Jan 2, 2025

Ok, the PR to remove the flag has been merged. Let me know if there's anything else we can do to help.

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

No branches or pull requests

3 participants