Exposing PropertyFetcher as a public API from OpenTelemetry package #1232

eddynaka · 2020-09-04T14:10:02Z

Fixes #1231.

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

codecov · 2020-09-04T14:17:00Z

Codecov Report

Merging #1232 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1232      +/-   ##
==========================================
- Coverage   78.93%   78.89%   -0.05%     
==========================================
  Files         219      215       -4     
  Lines        6347     6259      -88     
==========================================
- Hits         5010     4938      -72     
+ Misses       1337     1321      -16

Impacted Files	Coverage Δ
...c/OpenTelemetry/Instrumentation/PropertyFetcher.cs	`86.95% <ø> (ø)`
...tation.AspNetCore/Implementation/HttpInListener.cs	`90.52% <100.00%> (ø)`
...ent/Implementation/GrpcClientDiagnosticListener.cs	`89.28% <100.00%> (ø)`
...mentation/RedisProfilerEntryToActivityConverter.cs	`94.59% <100.00%> (ø)`
...us/Implementation/PrometheusExporterEventSource.cs	`63.63% <0.00%> (-9.10%)`	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	`100.00% <0.00%> (+2.85%)`	⬆️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`80.95% <0.00%> (+3.17%)`	⬆️

cijothomas · 2020-09-04T14:27:12Z

Add this to SDK changelog as a new adition to public API

eddynaka · 2020-09-04T14:43:22Z

Add this to SDK changelog as a new adition to public API

Just added

src/OpenTelemetry/Instrumentation/PropertyFetcher.cs

CodeBlanch

LGTM

reyang · 2020-09-04T16:40:38Z

I understand that we want to reduce duplicated code.
Is this something that we can achieve by having a set of common files referenced by multiple projects?
Is there a strong reason to expose a public API?

For example, we already have these shared files:

    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ExceptionExtensions.cs" Link="Internal\ExceptionExtensions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Trace\SemanticConventions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\EnumerationHelper.cs" Link="Implementation\EnumerationHelper.cs" />

reyang

I'm concerned about exposing this as a public function.
We probably need to consider the file sharing approach.

cijothomas · 2020-09-04T16:45:18Z

I understand that we want to reduce duplicated code.
Is this something that we can achieve by having a set of common files referenced by multiple projects?
Is there a strong reason to expose a public API?

For example, we already have these shared files:
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ExceptionExtensions.cs" Link="Internal\ExceptionExtensions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Trace\SemanticConventions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\EnumerationHelper.cs" Link="Implementation\EnumerationHelper.cs" />

Avoiding code duplication is not the main reason for this PR. This allows anyone to write a new Instrumentation for DiagnosticSource instrumented libraries. (We already have the basic classes for subscription public, propertyfetcher is also required to fully enable on to write a instrumentation library))

eddynaka · 2020-09-04T16:46:43Z

I understand that we want to reduce duplicated code.
Is this something that we can achieve by having a set of common files referenced by multiple projects?
Is there a strong reason to expose a public API?
For example, we already have these shared files:
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ExceptionExtensions.cs" Link="Internal\ExceptionExtensions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Trace\SemanticConventions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\EnumerationHelper.cs" Link="Implementation\EnumerationHelper.cs" />
Avoiding code duplication is not the main reason for this PR. This allows anyone to write a new Instrumentation for DiagnosticSource instrumented libraries. (We already have the basic classes for subscription public, propertyfetcher is also required to fully enable on to write a instrumentation library))

Should i move or should I maintain ?

cijothomas · 2020-09-04T16:49:54Z

I understand that we want to reduce duplicated code.
Is this something that we can achieve by having a set of common files referenced by multiple projects?
Is there a strong reason to expose a public API?
For example, we already have these shared files:
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ExceptionExtensions.cs" Link="Internal\ExceptionExtensions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Trace\SemanticConventions.cs" />
    <Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\EnumerationHelper.cs" Link="Implementation\EnumerationHelper.cs" />
Avoiding code duplication is not the main reason for this PR. This allows anyone to write a new Instrumentation for DiagnosticSource instrumented libraries. (We already have the basic classes for subscription public, propertyfetcher is also required to fully enable on to write a instrumentation library))
Should i move or should I maintain ?

Please wait to hear Reiley's thoughts after I made clarification above about the intent of this PR.

The following class, along with propertyfetcher is public - so that anyone can write their own instrumentation for DiagnosticSource instrumented libraries.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Instrumentation/ListenerHandler.cs

reyang · 2020-09-04T16:53:21Z

Avoiding code duplication is not the main reason for this PR. This allows anyone to write a new Instrumentation for DiagnosticSource instrumented libraries. (We already have the basic classes for subscription public, propertyfetcher is also required to fully enable on to write a instrumentation library))

Should i move or should I maintain ?

Please wait to hear Reiley's thoughts after I made clarification above about the intent of this PR.

The following class, along with propertyfetcher is public - so that anyone can write their own instrumentation for DiagnosticSource instrumented libraries.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Instrumentation/ListenerHandler.cs

I'm fine with exposing a public API if the pros > cons.
I think we need to by crystal clear on the PR title (the title should reflect the intention) "why we are making the change", in this case it shouldn't be "Removing duplicated PropertyFetcher" and it might need to be "Exposing PropertyFetcher as a public API from OpenTelemetry package".

eddynaka · 2020-09-04T18:12:23Z

Avoiding code duplication is not the main reason for this PR. This allows anyone to write a new Instrumentation for DiagnosticSource instrumented libraries. (We already have the basic classes for subscription public, propertyfetcher is also required to fully enable on to write a instrumentation library))

Should i move or should I maintain ?

Please wait to hear Reiley's thoughts after I made clarification above about the intent of this PR.
The following class, along with propertyfetcher is public - so that anyone can write their own instrumentation for DiagnosticSource instrumented libraries.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Instrumentation/ListenerHandler.cs

I'm fine with exposing a public API if the pros > cons.
I think we need to by crystal clear on the PR title (the title should reflect the intention) "why we are making the change", in this case it shouldn't be "Removing duplicated PropertyFetcher" and it might need to be "Exposing PropertyFetcher as a public API from OpenTelemetry package".

just updated the title!

CodeBlanch · 2020-09-04T20:16:47Z

src/OpenTelemetry/Instrumentation/PropertyFetcher.cs

 {
-    internal class PropertyFetcher
+    public class PropertyFetcher


nit: Should have XML comments now that it is public.

added some. not sure if the best text...

CodeBlanch · 2020-09-04T20:25:10Z

src/OpenTelemetry/Instrumentation/PropertyFetcher.cs

 {
-    internal class PropertyFetcher
+    public class PropertyFetcher


I think we could improve this API. Internally it has a very nice, typed delegate created. But we only expose object Fetch(object) which basically throws away that typing information and will box any value types + require a cast. With a little work we could probably also expose:

class PropertyFetcher<TType, TPropertyType> : PropertyFetcher { public TPropertyType Fetch(TType instance) }

That would be useful when you know the types ahead of time, which is actually the more common case. It would allow us to avoid some of those performance penalties.

Let's merge this and then we can try to improve it?

sounds great :)

@CodeBlanch not sure how it was done originally, but inside the same class, we already have a typedPropertyFetcher. but, in the end, we are just returning as an object.

…/eddynaka/opentelemetry-dotnet into feature/removing-duplicated-code

lmolkova · 2020-09-04T20:50:46Z

I remember back in the day we discussed (in scope of DiagnosticSource work) that exposing efficient property fetching APIs may be reasonable in .NET.
cc @noahfalk @tarekgh

Removing duplicated PropertyFetcher

e27b7af

eddynaka requested a review from a team September 4, 2020 14:10

cijothomas approved these changes Sep 4, 2020

View reviewed changes

Adding PropertyFetcher to changelog

55ffd4f

alanwest reviewed Sep 4, 2020

View reviewed changes

src/OpenTelemetry/Instrumentation/PropertyFetcher.cs Show resolved Hide resolved

CodeBlanch approved these changes Sep 4, 2020

View reviewed changes

reyang requested changes Sep 4, 2020

View reviewed changes

reyang self-requested a review September 4, 2020 17:02

eddynaka changed the title ~~Removing duplicated PropertyFetcher~~ Exposing PropertyFetcher as a public API from OpenTelemetry package Sep 4, 2020

reyang approved these changes Sep 4, 2020

View reviewed changes

Merge branch 'master' into feature/removing-duplicated-code

77de330

CodeBlanch reviewed Sep 4, 2020

View reviewed changes

eddynaka added 2 commits September 4, 2020 17:50

adding docs

9fa0c05

Merge branch 'feature/removing-duplicated-code' of https://github.com…

66122c6

…/eddynaka/opentelemetry-dotnet into feature/removing-duplicated-code

eddynaka and others added 2 commits September 5, 2020 18:27

Merge branch 'master' into feature/removing-duplicated-code

89d999a

Merge branch 'master' into feature/removing-duplicated-code

85c365e

CodeBlanch merged commit d0d33ed into open-telemetry:master Sep 6, 2020

CodeBlanch mentioned this pull request Sep 6, 2020

PropertyFetcher API Improvements #1238

Merged

3 tasks

eddynaka deleted the feature/removing-duplicated-code branch September 19, 2020 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposing PropertyFetcher as a public API from OpenTelemetry package #1232

Exposing PropertyFetcher as a public API from OpenTelemetry package #1232

eddynaka commented Sep 4, 2020

codecov bot commented Sep 4, 2020 •

edited

Loading

cijothomas commented Sep 4, 2020

eddynaka commented Sep 4, 2020

CodeBlanch left a comment

reyang commented Sep 4, 2020 •

edited

Loading

reyang left a comment

cijothomas commented Sep 4, 2020

eddynaka commented Sep 4, 2020

cijothomas commented Sep 4, 2020

reyang commented Sep 4, 2020

eddynaka commented Sep 4, 2020

CodeBlanch Sep 4, 2020

eddynaka Sep 4, 2020

CodeBlanch Sep 4, 2020

eddynaka Sep 4, 2020

eddynaka Sep 4, 2020

lmolkova commented Sep 4, 2020

Exposing PropertyFetcher as a public API from OpenTelemetry package #1232

Exposing PropertyFetcher as a public API from OpenTelemetry package #1232

Conversation

eddynaka commented Sep 4, 2020

Changes

codecov bot commented Sep 4, 2020 • edited Loading

Codecov Report

cijothomas commented Sep 4, 2020

eddynaka commented Sep 4, 2020

CodeBlanch left a comment

Choose a reason for hiding this comment

reyang commented Sep 4, 2020 • edited Loading

reyang left a comment

Choose a reason for hiding this comment

cijothomas commented Sep 4, 2020

eddynaka commented Sep 4, 2020

cijothomas commented Sep 4, 2020

reyang commented Sep 4, 2020

eddynaka commented Sep 4, 2020

CodeBlanch Sep 4, 2020

Choose a reason for hiding this comment

eddynaka Sep 4, 2020

Choose a reason for hiding this comment

CodeBlanch Sep 4, 2020

Choose a reason for hiding this comment

eddynaka Sep 4, 2020

Choose a reason for hiding this comment

eddynaka Sep 4, 2020

Choose a reason for hiding this comment

lmolkova commented Sep 4, 2020

codecov bot commented Sep 4, 2020 •

edited

Loading

reyang commented Sep 4, 2020 •

edited

Loading