-
Notifications
You must be signed in to change notification settings - Fork 312
feat(config): Max event handling #1103
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1103 +/- ##
===========================================
+ Coverage 78.95% 79.06% +0.11%
===========================================
Files 159 159
Lines 14919 14928 +9
===========================================
+ Hits 11779 11803 +24
+ Misses 2727 2713 -14
+ Partials 413 412 -1 see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| func TestConfigurableTxnEvents_configMoreThanMax(t *testing.T) { | ||
| h, err := internal.UnmarshalConnectReply([]byte( | ||
| `{"return_value":{ | ||
| "event_harvest_config": { | ||
| "report_period_ms": 10000 | ||
| } | ||
| }}`), internal.PreconnectReply{}) | ||
| if nil != err { | ||
| t.Fatal(err) | ||
| } | ||
| cfg := config{Config: defaultConfig()} | ||
| cfg.TransactionEvents.MaxSamplesStored = internal.MaxTxnEvents + 100 | ||
| result := newAppRun(cfg, h).MaxTxnEvents() | ||
| if result != internal.MaxTxnEvents { | ||
| t.Errorf("Unexpected max number of txn events, expected %d but got %d", internal.MaxTxnEvents, result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this test removed?
| // of how many custom events are stored in an agent for a given harvest cycle. | ||
| // Alters the CustomInsightsEvents.MaxSamplesStored setting. | ||
| // Note: As of Jul 2022, the absolute maximum events that can be sent each minute is 100000. | ||
| // Note: As of Oct 2025, the absolute maximum events that can be sent each minute is 30000. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec for custom insights states that the maximum number of events is 100000, not 30000
| // of how many transaction events are stored in an agent for a given harvest cycle. | ||
| // Alters the TransactionEvents.MaxSamplesStored setting. | ||
| // Note: As of Oct 2025, the absolute maximum events that can be sent each minute is 10000. | ||
| func ConfigTransactionEventsMaxSamplesStored(limit int) ConfigOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the variable limit in this context is a little confusing to me- at a glance, it implies that this function is receiving some kind of defined, canonical limit value rather than a user-desired config value for these settings. We may want to consider renaming this variable name to something like configured or just value while we're already changing the functions.
| } | ||
| } | ||
|
|
||
| func TestConfigMaxTxnEvents(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any other tests that validate the defaultConfig() function call behavior to set the cfg to the internal limits? I think we still tests to verify that the default config values behave as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is specific to Txn events but all event types should have at least a single test to verify that the default value exists when nothing is explicitly set and the values match expectations.
Links
Details
Issue
There is currently no way to locally configure the maximum for Transaction Events, Custom Insights Events, Error Events, and Log Events. Additionally, there are many functions that are essentially copied and renamed. There is also no handling of invalid config values at the time of setting (but rather later)
Goals
Implementation
ConfigTransactionEventsMaxSamplesStoredConfigTransactionEventsMaxSamplesStored,ConfigCustomInsightsEventsMaxSamplesStored,ConfigErrorCollectorMaxSamplesStored, andConfigAppLogForwardingMaxSamplesStorednow use functionmaxConfigEvents(limit, max)to handle invalid configurations when being setNEW_RELIC_TRANSACTION_EVENTS_MAX_SAMPLES_STOREDandNEW_RELIC_ERROR_COLLECTOR_MAX_EVENT_SAMPLES_STOREDNEW_RELIC_APPLICATION_LOGGING_FORWARDING_MAX_SAMPLES_STORED,NEW_RELIC_TRANSACTION_EVENTS_MAX_SAMPLES_STORED,NEW_RELIC_ERROR_COLLECTOR_MAX_EVENT_SAMPLES_STORED, andNEW_RELIC_CUSTOM_INSIGHTS_EVENTS_MAX_SAMPLES_STOREDnow useassignIntWithMax(field, name, max)when being assigned to handle invalid configurationsmaxCustomEvents,maxTxnEvents, andmaxLogEventsas their logic was moved tomaxConfigEventsand is called during assignment of config variablesMaxTxnEvents,MaxCustomEvents,MaxLogEvents,MaxSpanEvents, andMaxErrorEventsbecause they were all making a call torun.limitrun.limitrun.limitdirectly passes in the config values instead of functions to get a valid valueHow to test
More tests to come...
make core-suite