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.

Sign up for GitHub

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

Jump to bottom

Further configuration refactorings #2055

Merged
merged 19 commits into from
May 11, 2023
Merged

Further configuration refactorings #2055

merged 19 commits into from
May 11, 2023

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Apr 24, 2023 •
edited
Loading

  • Refactor configuration options to enums (ConfigurationOption and DynamicConfigurationOption) and make all configuration lookups take the enum instead of strings (keyname/variable).

  • IConfigurationReader now implements IConfigurationDescription, IConfigurationLookup

    • This allows us to assume we can get the log configuration on application start
    • This allows ConfigurationSnaphot to always get a Description

Breaking changes:

  • This removes ConfigConst.KeyNames and ConfigConst.EnvVarNames
  • IConfigurationReader now implements IConfigurationDescription, IConfigurationLookup

Mpdreamz and others added 14 commits April 19, 2023 18:28
@Mpdreamz
Eager load configuration readers.
977acae 
Removes all code from the getters and eagerly loads configuration in the constructors.

 This change was sparked by the stackoverflow discovered in ECS.NET where we log ServiceVersion but the getter itself also logs: elastic/ecs-dotnet#292

 This further simplifies caching and removes all lazy loading including our own `LazyContextualInit` construct.
@Mpdreamz
LazyContextualInit still used by ElasticApmModule leave it in for now.
7d7aad0
@Mpdreamz
Further refactorings:
c0e7fb5 
- Rename FullFrameworkConfigReader to ElasticApmModuleConfiguration
- Rename MicrosoftExtensionsConfig to ApmConfiguration

- Merge ConfigurationSnapshotFromReader and WrappingConfiguration into one `RuntimeConfigurationSnapshot`
@Mpdreamz
update AppSettingsTests
249b029
@Mpdreamz
Ensure ElasticApmModuleConfiguration eager loads its custom ServiceNa…
666088a 
…me resolution as well
@Mpdreamz
Ensure ElasticApmModuleConfiguration sanitizes service name it discovers
5e80d5a
@Mpdreamz
introduce more explicit configuration defaults for base classes
8a871b3
@Mpdreamz
fix dropped off }
73c982c
@Mpdreamz
Merge branch 'main' into fix/eager-config-load
c4a7b86
@Mpdreamz
remove code duplication from EnvironmentConfiguration
de9063c
@Mpdreamz
Enforce configuration impemlementations provide a description to be u…
de4940b 
…sed for snapshotting
@Mpdreamz
MockConfiguration relies on same assignment logic used by production …
fda2a37 
…ApmConfiguration and ElasticModuleConfiguration
@Mpdreamz
Refactor configuration options to enums and make all configuration lo…
c2096e4 
…okups more strongly typed
@Mpdreamz
Clean up how configuration descriptions are set
508ae4a
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Apr 24, 2023
@Mpdreamz Mpdreamz changed the base branch from main to fix/eager-config-load April 24, 2023 11:23
@Mpdreamz Mpdreamz added the breaking change A change or proposal that breaks the public API or any existing functionality label Apr 24, 2023
@apmmachine
Copy link
Collaborator

apmmachine commented Apr 24, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-05-11T14:05:16.872+0000

  • Duration: 66 min 55 sec

Test stats 🧪

Test Results
Failed 0
Passed 2538
Skipped 33
Total 2571

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Base automatically changed from fix/eager-config-load to main April 25, 2023 07:20
@Mpdreamz Mpdreamz mentioned this pull request Apr 26, 2023
Merged
@Mpdreamz Mpdreamz mentioned this pull request May 10, 2023
Closed
stevejgordon
stevejgordon approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

LGTM!

@Mpdreamz Mpdreamz merged commit acc14c8 into main May 11, 2023
APM-Agents (OLD) automation moved this from In Progress to Done May 11, 2023
@Mpdreamz Mpdreamz deleted the refactor/config-options branch May 11, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevejgordon stevejgordon stevejgordon approved these changes

Assignees
No one assigned
Labels
agent-dotnet breaking change A change or proposal that breaks the public API or any existing functionality
Projects
APM-Agents (OLD)
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mpdreamz @apmmachine @stevejgordon