Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 4 commits
232ab2b
62779b2
8da9a2d
6d791de
8973699
3961147
5d71484
660c721
c013789
4f7668d
813df21
ce8c48c
dc516aa
d241178
4352fc1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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).
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 referenceInstrumentation
here. As far as I know you cannot define a namespace with these top level statementsThere 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'?
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.
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:
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.
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
?ActivitySource
andMeter
. Is this a concern?opentelemetry-dotnet/docs/metrics/getting-started/Program.cs
Lines 25 to 26 in c0f927f
opentelemetry-dotnet/examples/Console/TestPrometheusExporter.cs
Lines 27 to 28 in c0f927f
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
andMeter
has been added.