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

FFM-11470 Fix analytics panic with nil target attributes #159

Merged
merged 12 commits into from
May 13, 2024
Merged

Conversation

erdirowlands
Copy link
Contributor

@erdirowlands erdirowlands commented May 13, 2024

What

The analytics code was refactored in #150 , and a nil check for target attributes was accidentally removed. This meant if an evaluation was made with a target with no attributes, we would dereference a nil pointer and a panic would occur when analytics are processed:

Screenshot 2024-05-13 at 15 05 34

This PR:

  • Adds a nil check before dereferencing target attributes

  • Extracts out evaluation/target metrics processing code into separate methods, so that they can be unit tested

  • Adds unit tests for the newly extracted methods, and sendDataAndResetCache

  • Also retracts [v0.1.21, v0.1.22] due to this bug being critical and easily encountered.

  • Bumps golang.org/x/net v0.25.0 for CVE-2023-45288

Testing

  • New unit tests to verify that metrics are processed correctly, and a panic does not occur if attributes or anonymous is nil.
  • New unit tests to verify that metrics are sent correctly.
  • Manual integration test with prod2 account.
  • TestGrid integration test.


targetAnalytics.iterate(func(key string, target evaluation.Target) {
targetAttributes := make([]metricsclient.KeyValue, 0)
if target.Attributes != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the nil check that was missing that would result in a panic.

@erdirowlands erdirowlands merged commit cb60b60 into main May 13, 2024
3 checks passed
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