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

[ti_cif3] Clean up null handling #9148

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

chrisberkhout
Copy link
Contributor

Proposed commit message

[ti_cif3] Clean up null handling (#)

- Remove redundant null-safe access to ctx.
- Make access after null-safe access also be null-safe.
- Combine 'not null and is/not value' checks.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Related issues

@chrisberkhout chrisberkhout added Team:Service-Integrations Label for the Service Integrations team bugfix Pull request that fixes a bug issue Integration:ti_cif3 Collective Intelligence Framework v3 labels Feb 14, 2024
@chrisberkhout chrisberkhout self-assigned this Feb 14, 2024
@chrisberkhout chrisberkhout requested a review from a team as a code owner February 14, 2024 14:54
@chrisberkhout
Copy link
Contributor Author

/test

@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@@ -124,13 +124,13 @@ processors:
field: cif3.indicator
target_field: threat.indicator.network.cidr
ignore_missing: true
if: "ctx.threat?.indicator?.type != null && ['ipv4-addr', 'ipv6-addr'].contains(ctx.threat.indicator.type) && (ctx.cif3?.indicator_ipv4_mask != null || ctx.cif3?.indicator_ipv6_mask != null)"
if: "['ipv4-addr', 'ipv6-addr'].contains(ctx.threat?.indicator?.type) && (ctx.cif3?.indicator_ipv4_mask != null || ctx.cif3?.indicator_ipv6_mask != null)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if in the cases where the field checks are obviously cheaper, they should not go first to avoid the futile array allocation for the Array.contains call. The case here and below are clear since it's a null check; the case above with a string compare may be less obvious, but also may benefit since a small string compare should be cheaper than an alloc.

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 did the case above in its own commit.

I did this case and below in a separate commit. These ones I'm ambivalent about because while it makes sense for performance, I think readability is better with the repeated condition first and the differentiating condition following.

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @chrisberkhout

Copy link

@chrisberkhout chrisberkhout merged commit 2264424 into elastic:main Feb 19, 2024
5 checks passed
@chrisberkhout chrisberkhout deleted the ti_cif3-null-handling branch February 19, 2024 11:07
@elasticmachine
Copy link

Package ti_cif3 - 1.10.2 containing this change is available at https://epr.elastic.co/search?package=ti_cif3

gizas pushed a commit that referenced this pull request Mar 13, 2024
- Remove redundant null-safe access to ctx.
- Make access after null-safe access also be null-safe.
- Combine 'not null and is/not value' checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug issue Integration:ti_cif3 Collective Intelligence Framework v3 Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants