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

Drop attributes with a value of None #1064

Merged
merged 9 commits into from
Feb 22, 2024
Merged

Drop attributes with a value of None #1064

merged 9 commits into from
Feb 22, 2024

Conversation

hmstepanek
Copy link
Contributor

@hmstepanek hmstepanek commented Feb 16, 2024

Overview

This PR attempts to align the Python agent with the agent spec by dropping attributes with a value of None.

According to the spec:

Agents SHOULD NOT report null attribute values. The behavior of IS NULL queries in insights makes it so that omitted keys behave the same as null keys. Since there is no difference between omitting the key and sending a null, we SHOULD reduce the payload size by omitting null values from the payload entirely.

Since there should not be a difference in behavior for customers recording null attributes versus customers omitting an attribute, agents SHOULD add debug logging when a null value is recorded. This will give agents observability on recording of null attributes outside of audit logs.

"empty" values such as "" and 0 MUST be sent as an attribute in the payload.

It also performs some minor cleanup and adds descriptions to some function inside our attribute filtering system.
It also contains a fix for previously failing sklearn tests due to a non integer version: 0.0.post12.

Recommended review strategy: review each commit one at a time.

@hmstepanek hmstepanek changed the base branch from main to develop-ai-limited-preview-3 February 16, 2024 03:42
Copy link

github-actions bot commented Feb 16, 2024

🦙 MegaLinter status: ❌ ERROR

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON bandit 6 0 5.4s
✅ PYTHON black 14 0 0 1.92s
✅ PYTHON flake8 14 0 1.04s
✅ PYTHON isort 14 0 0 0.33s
❌ PYTHON pylint 14 30 7.53s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@hmstepanek hmstepanek force-pushed the drop-none-attrs branch 2 times, most recently from eaf843c to e104b53 Compare February 16, 2024 04:08
@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6fd639d) 76.36% compared to head (03c1f50) 76.37%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1064   +/-   ##
=======================================
  Coverage   76.36%   76.37%           
=======================================
  Files         190      190           
  Lines       19747    19748    +1     
  Branches     3468     3470    +2     
=======================================
+ Hits        15080    15082    +2     
  Misses       3656     3656           
+ Partials     1011     1010    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hmstepanek hmstepanek changed the title Drop attributes that have a value of None Drop attributes with a value of None Feb 16, 2024
@hmstepanek hmstepanek changed the base branch from develop-ai-limited-preview-3 to main February 16, 2024 17:30
@hmstepanek hmstepanek marked this pull request as ready for review February 16, 2024 22:55
@hmstepanek hmstepanek requested a review from a team as a code owner February 16, 2024 22:55
@hmstepanek hmstepanek force-pushed the drop-none-attrs branch 2 times, most recently from 4039d06 to bb818f3 Compare February 20, 2024 21:55
Copy link
Contributor

@TimPansino TimPansino left a comment

Choose a reason for hiding this comment

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

Left comments, I've reviewed all the commits and it looks mostly good overall.

hmstepanek and others added 9 commits February 22, 2024 12:29
Now we can remove the check for None inside resolve_user_attributes since
process_user_attribute is always called on the attributes before they are passed to
resolve_user_attributes. This None check is unreachable code and not needed.
create_user_attributes is a passthrough to create_attributes. Just remove this function,
there's no need for it.
@hmstepanek hmstepanek merged commit 56fbda1 into main Feb 22, 2024
39 of 49 checks passed
@hmstepanek hmstepanek deleted the drop-none-attrs branch February 22, 2024 20:37
@mergify mergify bot removed the tests-failing label Feb 22, 2024
@umaannamalai umaannamalai added this to the v9.7.0 milestone Feb 22, 2024
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.

None yet

4 participants