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

Agent.Configuration now always points to ConfigurationStore's configuration #2109

Merged
merged 4 commits into from Jul 25, 2023

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Jun 6, 2023

This ensure no matter how you get to the configuration you will always get the latest version e.g one that is possibly updated through remote config.

This also deprecates .ConfigurationReader property on IApmAgent in favor of .Configuration.

@Mpdreamz Mpdreamz requested a review from gregkalapos July 5, 2023 13:02
Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

One thing I was thinking about when I looked at this: one concept we followed in the .NET Agent (and most other agents do this as well) is that a config snapshot is immutable for a given transaction. Meaning the config system takes all the values at the beginning of the transaction and those don't change until the end of the transaction. If central config changes during the transaction, then the new value only applies to the next transaction. The idea behind this is simply consistency and to decrease complexity (it's much easier to not deal with question like: what if I disable the agent in the middle of a transaction? What if I disable stack trace capturing in the middle of a transaction? etc. - the answer to all those is that the already started transaction always finishes with the original configsnapshot).

From what I see that's still true. We should just make sure to only access Agent.Configuration directly if it's not something which should be immutable during a transaction.

Sorry for the noise here, just wanted to share this.

@Mpdreamz Mpdreamz merged commit 9c393a5 into main Jul 25, 2023
14 checks passed
APM-Agents (OLD) automation moved this from In Progress to Done Jul 25, 2023
@Mpdreamz Mpdreamz deleted the fix/agent-configuration-property branch July 25, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants