Fix inconsistencies around harvest limit config settings

[hmstepanek]

Overview

• Fix serverside ml_event harvest limits ordering issue.
  • Previously the ml_event would not be added to the allow list which would reject ml_events from being recorded.
• Remove logging an error message when we attempt to delete a setting on the settings object that doesn't exist.
• Only report non-deprecated harvest limit setting names in log.

[github-actions]

✅MegaLinter analysis: Success

Descriptor	Linter	Files	Fixed	Errors	Warnings	Elapsed time
✅ ACTION	actionlint	8		0	0	1.06s
✅ MARKDOWN	markdownlint	7	0	0	0	1.42s
✅ PYTHON	ruff	1037	0	0	0	1.25s
✅ PYTHON	ruff-format	1037	0	0	0	0.4s
✅ YAML	prettier	20	0	0	0	1.68s
✅ YAML	v8r	20		0	0	5.12s
✅ YAML	yamllint	20		0	0	0.74s

See detailed reports in MegaLinter artifacts

Show us your support by starring ⭐ the repository

[codecov-commenter]

Codecov Report

❌ Patch coverage is 82.22222% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.00%. Comparing base (949339a) to head (dc565fe).

Files with missing lines	Patch %	Lines
newrelic/config.py	82.50%	5 Missing and 2 partials ⚠️
newrelic/core/config.py	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1737      +/-   ##
==========================================
- Coverage   82.03%   82.00%   -0.03%     
==========================================
  Files         215      215              
  Lines       26309    26324      +15     
  Branches     4150     4153       +3     
==========================================
+ Hits        21582    21588       +6     
- Misses       3312     3320       +8     
- Partials     1415     1416       +1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[TimPansino]

There are 3 issues in this PR that were surfaced by hand testing.

1. Log messages now incorrectly use the setting value instead of setting names.

The changes incorrectly assume truthiness is a valid way of comparing settings, when a literal 0 is a valid setting with a real meaning. This causes the following 2 issues.

2. Precedence is being ignored where the deprecated setting can win over the new setting if the new setting is set to 0.
3. Log messages detailing which settings are in use are incorrect if either is set to 0.

As part of the review, I added 2 test cases to the existing tests to validate that the value of 0 is being respected over both the default settings, and the deprecated settings. I pushed that in a new commit.

[TimPansino]

Another issue worth considering, we seem to have missed the setting for ml_event_data. We still use the old naming convention entirely for that setting. Would it be worth changing event_harvest_config.harvest_limits.ml_event_data -> ml_events.max_samples_stored for consistency?

That would require moving a fair bit of code around to fix, so maybe it would be best for a follow up. The other problem with this is noise for customers who have it set, but this setting is incredibly rare in the wild.

[TimPansino]

A note on the PR description:

Previously the ml_event would not be added to the allow list which would reject ml_events from being recorded.

The ml_event_data was never expected to be in the allowlist before, and we don't ever check the presence of it in the allowlist. Adding it there only makes it show up in the UI under the agent settings now, which is less confusing.

It's still not checked to this day, as the reset_ml_events function is actually gated by custom_event_data in the allow list, as this was originally an extension onto custom events.

The customer who originally reported the missing method from the allowlist was upset about their LLM data not being visible in the UI. That data does not have any relevance to the ml_event_data settings, as that's exclusively used for SKLearn instrumentation and ML happening on host. (That customer's data was also being reported in the first place as we found the historical data. They were having an issue with the UI tab not appearing which was later fixed without our intervention.)

[TimPansino]

Approving, with the note that the ml_event_data setting name is out of sync with the others but that's out of scope for this PR. We'll likely want to change that in a future major version.