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

Disable Sending of ML Events by Default #923

Merged
merged 8 commits into from
Sep 20, 2023

Conversation

umaannamalai
Copy link
Contributor

This PR sets the default value of ml_insights_events.enabled to False so it can be intentionally enabled by users who would like to send ML events to NR.

@umaannamalai umaannamalai requested a review from a team September 18, 2023 16:55
@github-actions
Copy link

github-actions bot commented Sep 18, 2023

🦙 MegaLinter status: ❌ ERROR

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON bandit 1 0 5.12s
✅ PYTHON black 6 5 0 1.45s
❌ PYTHON flake8 6 2 0.54s
✅ PYTHON isort 6 5 0 0.23s
❌ PYTHON pylint 6 9 4.16s

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

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023

Codecov Report

Merging #923 (8e7dda3) into develop-scikitlearn (86f0df8) will not change coverage.
The diff coverage is 100.00%.

@@                 Coverage Diff                  @@
##           develop-scikitlearn     #923   +/-   ##
====================================================
  Coverage                81.74%   81.74%           
====================================================
  Files                      190      190           
  Lines                    19581    19581           
  Branches                  3401     3401           
====================================================
  Hits                     16006    16006           
  Misses                    2593     2593           
  Partials                   982      982           
Files Changed Coverage Δ
newrelic/core/config.py 94.21% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mergify mergify bot removed the tests-failing label Sep 19, 2023
tests/agent_features/test_ml_events.py Outdated Show resolved Hide resolved
@@ -717,7 +717,7 @@ def default_otlp_host(host):
_settings.transaction_events.attributes.include = []

_settings.custom_insights_events.enabled = True
_settings.ml_insights_events.enabled = True
_settings.ml_insights_events.enabled = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to stay false too? Or just the global override setting?

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 one was supposed to stay False since we check this as a standalone setting in calls to record_ml_event and not in conjunction with machine_learning.enabled.

@TimPansino TimPansino merged commit 3465cdd into develop-scikitlearn Sep 20, 2023
45 of 46 checks passed
@TimPansino TimPansino deleted the disable-ml-events-default branch September 20, 2023 17:27
@umaannamalai umaannamalai added this to the v9.1.0 milestone Sep 25, 2023
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.

3 participants