-
Notifications
You must be signed in to change notification settings - Fork 764
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
[examples] Add manual activities and custom metrics to ASP.NET Core example #4133
[examples] Add manual activities and custom metrics to ASP.NET Core example #4133
Conversation
Sorry I thought the CLA was already signed. Working with legal for approval |
|
||
public WeatherForecastController(ILogger<WeatherForecastController> logger) | ||
public WeatherForecastController(ILogger<WeatherForecastController> logger, ActivitySource activitySource, Meter meter) |
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.
@dnelson-relativity In the current shape of the .NET API I think it is an anti-pattern to use DI for ActivitySource & Meter. They are really intended to be used statically. If you really want to inject them, might be better to create a telemetry helper class which exposes source + meter for the app logic and can be safely injected.
/cc @noahfalk
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.
Happy to change this to static but I am curious if you could provide more context as to the reason why. I think the DI pattern is a bit more natural for an AspNetCore developer so that is what I chose. Perhaps the examples/docs should speak to that reasoning?
In this case is there something fundamentally different between using it as a singleton and using it statically?
This example is pretty simple so I can just new up the ActivitySource
/Meter
in WeatherForecastController
but I wouldn't want to make people think they should be doing that in every controller. Do you think that is ok or should I create these elsewhere?
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.
In this case is there something fundamentally different between using it as a singleton and using it statically?
The main issue conceptually is it is perfectly acceptable and expected there will many ActivitySource
s and many Meter
s in a process so who wins when multiple things call AddSingleton
? 😄 Here is what ASP.NET Core does so this code registering its own singleton is probably taking over ASP.NET Core's and causing some very odd telemetry!
This example is pretty simple so I can just new up the ActivitySource/Meter in WeatherForecastController but I wouldn't want to make people think they should be doing that in every controller. Do you think that is ok or should I create these elsewhere?
Probably best to keep it simple and just new them up in the controller. Best practice would probably be build something which plugs into MVC like an action filter (are those still a thing?) but that might be more code than it is worth for an example. It is very hard to show something useful without too much code in these docs/examples! If you are super passionate about it, we could have a more advanced example. Not sure how everyone would feel about that, but I'm not opposed to it.
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 see what you mean. If I update the TracerProvider
configuration to the following and disable the auto instrumentation
builder
.AddSource("Microsoft.AspNetCore", "Examples.AspNetCore")
.SetSampler(new AlwaysOnSampler());
//.AddAspNetCoreInstrumentation() -> Disabled
I get this unexpected result
Interestingly this doesn't happen with only the auto instrumentation which is why I never encountered this before.
I updated the example to use statics. On the fence about the "Examples.AspNetCore" magic string but it removes the need for some global constant or passing of additional configuration to the controller. Part of me thinks it is more explicit with the duplication.
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 don't know that it is written down anywhere, but we had past discussions with @davidfowl on this topic and agreed that:
- It was a mistake for ASP.NET to directly inject an instance of ActivitySource, but given back-compat concerns for now we are keeping the mistake as-is rather than changing it.
- In general developers shouldn't add types to a DI container if their code doesn't define the type. If you want to access an instance of some common type, a better pattern is to define a type that will contain it and inject the containing type. For example instead of injecting ActivitySource, define
class MyLibraryTelemetry { ActivitySource Source; }
and inject MyLibraryTelemetry instead. This avoids the type collisions with other components that might also like to locate an ActivitySource via the DI container.
private static readonly ActivitySource MyActivitySource = new ActivitySource("Examples.AspNetCore"); | ||
private static readonly Meter MyMeter = new Meter("Examples.AspNetCore"); |
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.
Why is anything static? I think we need a DI centric approach to counters here. This doesn't look like something we'd tell people to do.
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.
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.
Injecting the ActivitySource
or Meter
is bad but injecting a singleton service that has the instance of those types is good (that's what we should do here). With DI the type is the key so global singletons that the user doesn't own is a bad idea and easy to stomp on.
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.
@davidfowl looks like @noahfalk captured it here #4133 (comment).
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.
@CodeBlanch also hinted at this in his original comment. I am curious where you think the responsibility lies for creating the actual Instrument
(in this case a Counter
)
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.
Short answer - If the Meter is shared via a DI container you probably want the Instruments also shared via the DI container. Most likely this means the code that created the Meter is creating the instruments.
Long answer - Meters hold a reference to all Instruments they create, so once created Instruments live as long as their parent Meter does. Assuming the Meter object has a long lifetime that will be reused for an unbounded number of requests, creating new instances of Instrument objects per-request would leak memory so we don't want to do that. The alternatives that don't leak require all code using the Instrument objects to be sharing the same bounded set of them. This means either that the instruments are singletons in the container or they are stored in fields of some other DI singleton, or some other mechanism is employed to share them such as a static cache.
More broadly folks who have looked into using Meters in DI have found them lacking. I would argue none of the requests I have seen represent an inability for Meters to be used with a DI container, but people's expectations change and they expect more functionality or alternate patterns in addition to just putting the objects in DI. I think the best pattern you can do today is to put the Meter + all the Instruments in a container type and let DI inject it. As we look into some of the requests folks are making I expect some new APIs will show up and best practice will shift.
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 would recommend declaring singleton service that holds meter and activity source instances and flowing that to sample code.
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.
@noahfalk @davidfowl thank you for the feedback. I pushed an update that I hope aligns with your desires. I named the type TelemetryService
but maybe it should be TelemetryContainer
, TelemetryWrapper
, etc.
|
||
public TelemetryService(string version) | ||
{ | ||
this.ActivitySource = new ActivitySource(InstrumentationScopeName, version); |
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.
Couple things for discussion:
- Where do we dispose some of these properties?
- Do we want to make sure that at maximum 1 instance can exist? (rather than assuming that folks who use DI pattern would ensure it is singleton)?
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.
- A few comments regarding your first question
TelemetryService
orITelemetryService
(if the is registered likeappBuilder.Services.AddSingleton<ITelemetryService>
does the interface have to implement IDisposable?) could implementIDisposable
and I believe it will be handled on application shutdown. Is there a concern regarding order of disposal betweenActivitySource
andTracerProvider
orMeter
andMeterProvider
?- The examples that use statics don't dispose of
ActivitySource
andMeter
. Is this a concern?- https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/examples/MicroserviceExample/Utils/Messaging/MessageReceiver.cs#L29
opentelemetry-dotnet/docs/metrics/getting-started/Program.cs
Lines 25 to 26 in c0f927f
private static readonly Meter MyMeter = new("MyCompany.MyProduct.MyLibrary", "1.0"); private static readonly Counter<long> MyFruitCounter = MyMeter.CreateCounter<long>("MyFruitCounter"); opentelemetry-dotnet/examples/Console/TestPrometheusExporter.cs
Lines 27 to 28 in c0f927f
private static readonly Meter MyMeter = new("MyMeter"); private static readonly Meter MyMeter2 = new("MyMeter2");
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'd recommend TelemetryService implement IDisposable (it is not necessary for ITelemetryService to extend IDisposable if we still have the interface at all).
-
I don't think there is any concern on the relative disposal order, any order should work.
-
I don't think we should try to enforce that only one instance of this object exists. For a developer unit testing their own telemetry generation it may be quite useful to have multiple instances existing even if OTel wouldn't support transmitting the telemetry sent from those multiple instances. Longer term we probably want OTel to handle situations where an app may have multiple identically named Meters existing in the process within different DI containers. These Meters might be serviced by different instances of the OTel telemetry pipeline per container, or they might be tagged in some way to allow them to be consilidated in a single pipeline.
-
The reason the statics don't worry about disposal is they assume the lifetime of the Meter is the same as the lifetime of the process and the OS will release all resource on process exit. However in a DI container there is a presumption that the container might be shorter lived than the process (for example in a unit test).
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.
Disposal of ActivitySource
and Meter
has been added.
Co-authored-by: David Fowler <[email protected]>
using System.Diagnostics; | ||
using System.Diagnostics.Metrics; | ||
|
||
public interface ITelemetryService |
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.
@davidfowl may disagree, but creating an interface feels like unnecessary ceremony to me. Presumably this is an internal API boundary between two different components in the same library so I would not expect any alternate implementations to exist. I think the other usual argument is that someone might want to mock the interface for testing, but I doubt they could produce a useful mock unless the interface abstracted them from all the concrete types. If we need to delve into it I have some thoughts on how this can be tested without mocking.
If we do wind up keeping the interface I'd suggest not making it public.
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.
We don’t need an interface, just an implementation would suffice. Though, I think naturally people would expect to see an interface (even though there's nothing really mockable here).
using System.Diagnostics; | ||
using System.Diagnostics.Metrics; | ||
|
||
public class TelemetryService : ITelemetryService |
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'd recommend not appending 'Service' on this type name. All types that get added to a DI container can be refered to as services, but it is not usually added to the type name.
Naming nit: how about 'Instrumentation' instead of 'Telemetry'?
{ | ||
private const string InstrumentationScopeName = "Examples.AspNetCore"; | ||
|
||
public TelemetryService(string version) |
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'd recommend not having a version parameter here. Its not clear to me we are gaining any value by parameterizing it. We can inline typeof(TelemetryService).Assembly.GetName().Version?.ToString()
below.
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 parameterized it so that it was guaranteed to match the version defined as part of the provider's resource. Not that I would necessarily expect either of these to change.
Slightly related but I dislike how the original code was setting "unknown" in the case of null since it is an optional parameter and already handled nicely.
|
||
public TelemetryService(string version) | ||
{ | ||
this.ActivitySource = new ActivitySource(InstrumentationScopeName, version); |
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'd recommend TelemetryService implement IDisposable (it is not necessary for ITelemetryService to extend IDisposable if we still have the interface at all).
-
I don't think there is any concern on the relative disposal order, any order should work.
-
I don't think we should try to enforce that only one instance of this object exists. For a developer unit testing their own telemetry generation it may be quite useful to have multiple instances existing even if OTel wouldn't support transmitting the telemetry sent from those multiple instances. Longer term we probably want OTel to handle situations where an app may have multiple identically named Meters existing in the process within different DI containers. These Meters might be serviced by different instances of the OTel telemetry pipeline per container, or they might be tagged in some way to allow them to be consilidated in a single pipeline.
-
The reason the statics don't worry about disposal is they assume the lifetime of the Meter is the same as the lifetime of the process and the OS will release all resource on process exit. However in a DI container there is a presumption that the container might be shorter lived than the process (for example in a unit test).
|
||
public class Instrumentation : IDisposable | ||
{ | ||
internal const string ScopeName = "Examples.AspNetCore"; |
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.
Exposing this since the ActivitySource
and Meter
registration is coupled to their names.
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'd suggest to rename "ScopeName" to something else.
Scope is used in many context, so it could confuse new users.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#get-a-tracer
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.
Do you have a suggestion? In my opinion the instrumentation scope is the entity to which this data is associated. It also matches the underlying proto (granted most users probably won't know that)
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.
okay to use ActivitySourceName, MeterName.
AddSource, AddMeter usage is shown later, which looks easier to follow with AddMeter(instrumentaion.MeterNamer), AddActivitySource(instrument.ActivitySourceName)
|
||
public ActivitySource ActivitySource { get; } | ||
|
||
public Meter Meter { get; } |
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.
If we expect all Instrument
s to be created ahead of time is there any value with exposing Meter
?
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.
Nope, not really. You could remove it or make it private.
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 made it private. I could also see one making the argument that it should be static now 😂.
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.
Don't make it static :) Many devs who like DI patterns will immediately be worried if they see it.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main open-telemetry/opentelemetry-dotnet#4133 +/- ##
==========================================
- Coverage 85.59% 85.58% -0.01%
==========================================
Files 293 293
Lines 11371 11371
==========================================
- Hits 9733 9732 -1
- Misses 1638 1639 +1
|
|
||
// Manually create an activity. This will become a child of | ||
// the incoming request. | ||
using var activity = this.instrumentation.ActivitySource.StartActivity("calculate forecast"); |
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.
could we store the activitysource, and counter instead? so that this.Activitysource.StartActivity
, this.FreezingDaysCounter
style code can be used.
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.
Do you have a preference on the null checks in the constructor?
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 am just checking for Instrumentation
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.
oh I see I just made the opposite advice ;p @cijothomas what do you like about the multiple fields?
Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
eb9e94e
to
813df21
Compare
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.
LGTM!
Thanks for doing this and congrats on 1st PR :)
@@ -14,7 +14,7 @@ | |||
// limitations under the License. | |||
// </copyright> | |||
|
|||
using System.Reflection; | |||
using Examples.AspNetCore; |
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.
"using" or "namespace"?
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 think using
is correct. This allows me to reference Instrumentation
here. As far as I know you cannot define a namespace with these top level statements
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.
LGTM with minor suggestions (nothing blocking).
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.
Thanks @dnelson-relativity! Its looking a lot better and sorry to keep nit-picking. This is the first sample I've seen in any official documentation that suggests best-practices for using these APIs from an ASP.NET Core app so its getting above average scrutiny.
{ | ||
this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
|
||
ArgumentNullException.ThrowIfNull(instrumentation); | ||
this.activitySource = instrumentation.ActivitySource; |
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.
Nit: How about we store the Instrumentation object directly in a single field instead of one field per source and counter. It doesn't make a big difference for this sample, but for folks who have many counters it avoids adding lots of fields to their type if they apply the same pattern.
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 suggested this pattern to simply code elsewhere, but I can see merits to your suggestion too.
this.instrumentation.activitySource.StartActivity, this.instrumentation.counter.Add
vs
this.activitySource.StartActivity, this.counter.Add
Considering this as non-blocking, so proceeding to merge.
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.
Are you asking to revert this suggestion? I will let you and @cijothomas decide.
@@ -54,6 +74,9 @@ public IEnumerable<WeatherForecast> Get() | |||
}) | |||
.ToArray(); | |||
|
|||
// Optional: Count the freezing days | |||
this.freezingDaysCounter.Add(forecast.Count(f => f.TemperatureC < 0)); |
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.
People might be confused if they think about the meaning of this counter. Presumably if a web service is issuing weather reports it is probably giving reports for the same days over and over which means this counter will have a value something like num_requested_reports * num_freezing_days_per_report. How about a counter that tracks the number of forecasts instead? That value both seems more plausibly useful and makes more sense to use a Counter to track it.
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.
Good idea. We can add forecast as a dimension.
Lets address in a follow up.
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 was trying to find a "real" enough use case but didn't want to change the logic of the example. Would you define a forecast here as single invocation of this endpoint? In that case this information is the same as number of requests which one can get from the histogram created by AddAspNetCoreInstrumentation
.
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.
Yeah, each invocation of this method would be Add(1) in my suggestion. I agree it is duplicative most of the time (they might not have those other metrics enabled) but that still seemed like an improvement. If we wanted something that is both reasonable and unique we'd probably need to change the sample at least a little. For example the forecast service could have a cache and the counter counts how many requests weren't found in the cache. Another option could be forecast requests are for a variable number of days and the counter counts how many 10 day forecasts were requested (maybe 10 day forecasts are more expensive to produce than shorter forecasts?).
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 tried capturing this in a new issue (https://github.com/open-telemetry/opentelemetry-dotnet/issues/4166) so this does not get lost. If you feel extra clarity is needed please add it to the issue.
@dnelson-relativity Once last red CI check (sanitycheck/encoding one), and once its done, we can merge |
@cijothomas I think I fixed it. My Visual Studio keeps using CRLF for some reason. I know line endings are a contentious topic but is this something that should be added to the |
minor non-blocking comments could be a follow up. Merging to get this example ready by 1.4 release :) |
Is this example considered to be canonical? If so, it'd be great to copy things over to the official OTel docs and demo app: open-telemetry/opentelemetry.io#2024 Otherwise most people who instrument with otel-dotnet may not be following best practices. |
Yes. We expect all asp.net core examples to move to this model for manual span/metrics. |
Fixes #4107.
Changes
Please provide a brief description of the changes here.
ActivitySource
and add manual activityMeter
and add custom metric (counter)For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes