-
Notifications
You must be signed in to change notification settings - Fork 206
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
Conversation
…all components declare dependencies as constructor arguments rather than defering to Elastic.Apm.Agent
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
👍 My only comment to this is that I think most of the components will end up having |
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
src/Elastic.Apm.EntityFrameworkCore/ElasticEntityFrameworkCoreListener.cs
Outdated
Show resolved
Hide resolved
src/Elastic.Apm.EntityFrameworkCore/EfCoreDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
src/Elastic.Apm.AspNetCore/DiagnosticListener/AspNetCoreDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
Can confirm this works! 🍰 |
There was a problem hiding this 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!
…d expose a static Subscribe method on Agent
@Mpdreamz |
🚀🚀🚀 LGTM, Thx @Mpdreamz! 👍 |
This reworks
Elastic.Apm.Agent
making it static class holding a lazy singleton instance ofApmAgent
which isinternal
.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 theCreateLogger
andSetLoggerMethods()
. I refactored to make the tests pass but once #61 is settled on this could be cleaned up even further.