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

Custom Events Limiter #524

Merged
merged 10 commits into from Jun 29, 2022
Merged

Conversation

mirackara
Copy link
Contributor

@mirackara mirackara commented Jun 27, 2022

Description

Added configuration option for limiting the amount of samples stored in a custom insights event.

Usage

When creating a new application, use the following function to limit sample sizes

app, err := newrelic.NewApplication(
	newrelic.ConfigAppName("Short Lived App"),
	newrelic.ConfigLicense(os.Getenv("NEW_RELIC_LICENSE_KEY")),
	newrelic.ConfigCustomInsightsEventsMaxSamplesStored(240), // Configuration for limiting samples stored
)

@mirackara mirackara self-assigned this Jun 27, 2022
@CLAassistant
Copy link

CLAassistant commented Jun 27, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ mirackara
❌ Mirac Kara


Mirac Kara seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mirackara mirackara marked this pull request as ready for review June 27, 2022 21:09
@mirackara mirackara linked an issue Jun 27, 2022 that may be closed by this pull request
nr-swilloughby
nr-swilloughby previously approved these changes Jun 28, 2022
@iamemilio iamemilio self-requested a review June 29, 2022 15:59
Copy link
Contributor

@iamemilio iamemilio left a comment

Choose a reason for hiding this comment

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

This seems to be causing a number of the legacy tests to fail, which it should not effect. I think we should get to the bottom of why that is happening before we merge anything.

I noticed that some of your changes altered internal, rather than v3/internal. I suspect that a lot of your trouble is coming from that change.

@nr-swilloughby
Copy link
Contributor

Good catch!

@mirackara
Copy link
Contributor Author

Legacy tests are now good. Looks like some more of the v3 tests are failing however. Thanks for the comment!

@RichVanderwal
Copy link
Contributor

Thanks for looking at how the other agents are configured when you were naming this option!

@iamemilio
Copy link
Contributor

Looks a lot better. Once it passes tests, this is ready to squash and merge

@iamemilio iamemilio self-requested a review June 29, 2022 19:44
iamemilio
iamemilio previously approved these changes Jun 29, 2022
@iamemilio iamemilio merged commit ea1dcf9 into newrelic:develop Jun 29, 2022
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.

can not configure max samples stored for custom events
5 participants