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

Setting from NR backend should override setting from MAX_EVENT_SAMPLES_STORED and MAX_TRANSACTION_SAMPLES_STORED env var #1239

Merged
merged 8 commits into from
Sep 12, 2022

Conversation

vuqtran88
Copy link
Contributor

@vuqtran88 vuqtran88 commented Sep 8, 2022

Description

At the current state, when MAX_EVENT_SAMPLES_STORED env var is set, the agent uses value from the env var for the preconnect request. The server replies with a smaller value (whatver the value sent up get divided by 12). However, the agent won't use the value from the server because our current logic allows the env to override any config values. This causes configuration mismatched as well as incorrect ...\EvenHarvest\CustomEventData\HarvestLimit supportability metric value being reported.

The same story happens to the MAX_TRANSACTION_SAMPLES_STORED env var usage.

The right approach is to let the server config to dictate the final value that the agent will use.

@vuqtran88 vuqtran88 changed the title Setting from NR backend should override setting from MAX_EVENT_SAMPLES_STORED env var Setting from NR backend should override setting from MAX_EVENT_SAMPLES_STORED and MAX_TRANSACTION_SAMPLES_STORED env var Sep 8, 2022
@vuqtran88 vuqtran88 requested a review from JcolemanNR September 8, 2022 21:39
Copy link
Member

@nr-ahemsath nr-ahemsath left a comment

Choose a reason for hiding this comment

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

I'd like to add some clarifying comments to help the next person to look at this understand the precedence order of ServerOverrides vs. EnvironmentOverrides and why it is different from other cases.

vuqtran88 and others added 2 commits September 9, 2022 13:15
…on.cs

Co-authored-by: Alex Hemsath <57361211+nr-ahemsath@users.noreply.github.com>
…on.cs

Co-authored-by: Alex Hemsath <57361211+nr-ahemsath@users.noreply.github.com>
nr-ahemsath
nr-ahemsath previously approved these changes Sep 9, 2022
Copy link
Member

@nr-ahemsath nr-ahemsath left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@nr-ahemsath nr-ahemsath left a comment

Choose a reason for hiding this comment

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

Looks even better than before! :shipit:

@nr-ahemsath nr-ahemsath self-requested a review September 9, 2022 21:45
Copy link
Member

@nr-ahemsath nr-ahemsath left a comment

Choose a reason for hiding this comment

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

@vuqtran88 Josh reminded me that we need a changelog entry for this

Copy link
Member

@nr-ahemsath nr-ahemsath left a comment

Choose a reason for hiding this comment

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

Thanks for updating the changelog. 💯

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