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

Proposal for Elastic.Apm.Agent instantiation #63

Merged
merged 7 commits into from
Jan 14, 2019

Conversation

Mpdreamz
Copy link
Member

This reworks Elastic.Apm.Agent making it static class holding a lazy singleton instance of ApmAgent which is internal.

There is now an Elastic.Apm.Setup() method that allows you to control the lazy instantiation which will throw if the singleton materialized already and can no longer be configured. This is in favor of having too many (internal) static property setters.

This also allows all other components to explicitly declare their dependencies in the constructor without referring to the static singleton directly. To me this was a ServiceLocator in disguise and so this PR introduces several constructors.

All the tests now instantiate a local ApmAgent and can test in isolation.

Elastic.Apm.Agent no longer has all the CreateLogger and SetLoggerMethods(). I refactored to make the tests pass but once #61 is settled on this could be cleaned up even further.

…all components declare dependencies as constructor arguments rather than defering to Elastic.Apm.Agent
@codecov-io
Copy link

codecov-io commented Jan 10, 2019

Codecov Report

Merging #63 into master will decrease coverage by 2.93%.
The diff coverage is 66.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   78.44%   75.51%   -2.94%     
==========================================
  Files          30       35       +5     
  Lines         812      878      +66     
  Branches      100      108       +8     
==========================================
+ Hits          637      663      +26     
- Misses        135      173      +38     
- Partials       40       42       +2
Impacted Files Coverage Δ
src/Elastic.Apm/Config/AgentComponents.cs 0% <0%> (ø)
...astic.Apm/Config/EnvironmentConfigurationReader.cs 100% <100%> (ø)
src/Elastic.Apm/Model/Payload/Transaction.cs 97.31% <100%> (+0.04%) ⬆️
...re/DiagnosticListener/HttpDiagnosticsSubscriber.cs 100% <100%> (ø)
src/Elastic.Apm.AspNetCore/ApmMiddleware.cs 84.48% <100%> (-1.03%) ⬇️
....Apm/DiagnosticSource/HttpDiagnosticsSubscriber.cs 100% <100%> (ø)
...EntityFrameworkCore/EfCoreDiagnosticsSubscriber.cs 100% <100%> (ø)
src/Elastic.Apm/Config/ConfigurationKeyValue.cs 100% <100%> (ø)
...DiagnosticListener/AspNetCoreDiagnosticListener.cs 75% <100%> (-1.93%) ⬇️
src/Elastic.Apm/Api/Tracer.cs 96.58% <100%> (+0.67%) ⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a240d4...583d5dd. Read the comment docs.

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 very nice! I added one comment, other than that I think it's very neat! I'll take a closer look tomorrow.

@gregkalapos
Copy link
Contributor

gregkalapos commented Jan 11, 2019

This also allows all other components to explicitly declare their dependencies in the constructor without referring to the static singleton directly. To me this was a ServiceLocator in disguise and so this PR introduces several constructors.

👍 My only comment to this is that I think most of the components will end up having AbstractAgentConfig as the constructor parameter (will already happen here). Mainly because later we will have a bunch of settings on AbstractAgentConfig (see here), so components that now get these new constructors will typically need those (AbstractAgentConfig.[SomeConfigSetting]) and the AbstractAgentConfig.Logger. Should we maybe introduce a contract for this? Like adding an abstract IDiagnosticListener implementation that takes AbstractAgentConfig as parameter? (alternatively turn that into an abstract class)? That way we can avoid accidentally exploding the number of constructor parameters later.

@gregkalapos
Copy link
Contributor

Thanks to this change we can also remove the [assembly: CollectionBehavior(DisableTestParallelization = true)] lines here and here. Maybe we would have seen the flaky tests with that change.

@@ -12,8 +13,8 @@ public class ElasticCoreListeners
/// <summary>
/// Start listening for diagnosticsource events. Only listens for sources that are part of the Agent.Core package.
/// </summary>
public void Start() => DiagnosticListener
public void Start(AbstractLogger logger) => DiagnosticListener
Copy link
Contributor

@gregkalapos gregkalapos Jan 11, 2019

Choose a reason for hiding this comment

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

Can we default to Agent.Config.Logger (or Agent.Config) here and make that parameter optional? That way we would have a less complex public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved these listeners and start behind IDiagnosticSubscriber so the public contract folks can just provide us with an instance see e.g:

- app.UseElasticApm(Configuration);
- new ElasticCoreListeners().Start();
- new ElasticEntityFrameworkCoreListener().Start();
+ app.UseElasticApm(Configuration, new HttpDiagnosticsSubscriber(), new EfCoreDiagnosticsSubscriber());

or IApmAgent could also have Subscribe() to help hook this up even cleaner.

Copy link
Contributor

@gregkalapos gregkalapos Jan 11, 2019

Choose a reason for hiding this comment

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

Not sure we are ok here. The downside I think we still have is that once this is not started in the ApmMiddlewareExtension then activating those is still more complex then it should be - I'm not sure it's even possible now. Example scenario: .NET Core console application that wants to monitor outgoing HttpClient calls. In that case it would be nice to have the option to subscribe to those diagnostic source events (aka turn on outgoing HTTP monitoring) with a very simple 1 liner. And that was new ElasticCoreListeners().Start(); before this change. I think we should aim for something that is as simple as this one because it's a very basic feature and we can't be sure that we can turn this on automatically in all scenarios. Same applies to other diagnostic source related things (EFCore, etc..).

Update:

or IApmAgent could also have Subscribe() to help hook this up even cleaner.

Yes, I think that would be a nice way to do it!

@Mpdreamz
Copy link
Member Author

Thanks to this change we can also remove the [assembly: CollectionBehavior(DisableTestParallelization = true)] lines here and here. Maybe we would have seen the flaky tests with that change.

Can confirm this works! 🍰

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.

Thx for the changes! I added some more comments, looks very nice!

src/Elastic.Apm/Config/ConfigConsts.cs Outdated Show resolved Hide resolved
src/Elastic.Apm.AspNetCore/ApmMiddlewareExtension.cs Outdated Show resolved Hide resolved
src/Elastic.Apm.AspNetCore/ApmMiddlewareExtension.cs Outdated Show resolved Hide resolved
src/Elastic.Apm/Config/AgentComponents.cs Outdated Show resolved Hide resolved
src/Elastic.Apm/Config/AgentComponents.cs Show resolved Hide resolved
src/Elastic.Apm/Config/IAgentConfigReader.cs Outdated Show resolved Hide resolved
@gregkalapos
Copy link
Contributor

@Mpdreamz AspNetCoreDiagnosticListenerTest.TestErrorInAspNetCore fails because the AspNetCoreDiagnosticsSubscriber is not used. That would start AspNetCoreDiagnosticListener listening for errors within the ASP.NET Core pipeline.

@gregkalapos
Copy link
Contributor

🚀🚀🚀 LGTM, Thx @Mpdreamz! 👍

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