[Instrumentation.Remoting] initial implementation#3829
[Instrumentation.Remoting] initial implementation#3829lewis800 wants to merge 51 commits intoopen-telemetry:mainfrom
Conversation
|
|
|
Thanks for your contribution @lewis800. Before we get into reviewing the code in this PR, is this an instrumentation library you're willing to contribute and support for feature requests/issues going forwards if it were merged and released to NuGet.org? The maintainers only have a fixed amount of bandwidth and we can't take on the maintenance burden of every possible piece of new instrumentation that might be contributed to the repo, particularly if they're a more niche use case. |
|
Hi @martincostello for the foreseeable future I have the capacity to contribute and support this remoting instrumentation, I will be using it actively and I suspect likely, will be the one coming back with the requests and issues regarding it. |
|
@lewis800 Cool - in that case can you update the PR to make you a Code Owner in the appropriate places. For example:
The CI setup will also need updating in the relevant places to build, test and publish the new instrumentation as appropriate. You should be able to find all the relevant changes needed by searching the repo for other instrumentations' name and seeing where they appear. |
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
|
@martincostello I have updated the readme and think I have successfully added the remoting project into the existing workflows |
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/TracerProviderBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/TracerProviderBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/TracerProviderBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.Remoting.Tests/RemotingInstrumentationTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.Remoting.Tests/RemotingInstrumentationTests.cs
Show resolved
Hide resolved
Kielek
left a comment
There was a problem hiding this comment.
Could you please consider splitting this PR to 2 chunks? First - just project scaffolding with empty project, CI changes, etc. No classes should be fine.
It will be easier to review the code itslef.
For sure you are missing CHANGELOG.md near to README.md
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
test/OpenTelemetry.Instrumentation.Remoting.Tests/RemotingInstrumentationTests.cs
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.Remoting.Tests/RemotingInstrumentationTests.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/TracerProviderBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/TracerProviderBuilderExtensions.cs
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/RemotingInstrumentation.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSinkProvider.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Show resolved
Hide resolved
@lewis800, I see that you are working on infrastructure fixes here. This was my initial call to split it to separate PR. Cost of extraction does not seem so big to avoid this. |
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSinkProvider.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Martin Costello <martin@martincostello.com>
|
@martincostello @Kielek should be rebased off main, let me know if we need any further changes or clean up etc |
Kielek
left a comment
There was a problem hiding this comment.
I hope this comments make sense, please let me know what do you think.
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/TelemetryDynamicSink.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/OpenTelemetry.Instrumentation.Remoting.csproj
Show resolved
Hide resolved
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Extensions.Options" /> | ||
| <PackageReference Include="OpenTelemetry" /> |
There was a problem hiding this comment.
In general, there is a good practice to reference only OpenTelemetry.Api package by the instrumentations. The only downside is probably lack of support for Sdk.SuppressInstrumentation. I would not consider this as a blocker as WCF is doing probably the same thing.
There was a problem hiding this comment.
I only added Sdk.SuppressInstrumentation as I saw other instrumentation use it, so I am not set in stone on having / vs not having it. I'll leave it upto you to decide
There was a problem hiding this comment.
@Kielek I'm happy to just remove it for now and we can add it in back later if its needed, to get this PR merged for now
src/OpenTelemetry.Instrumentation.Remoting/RemotingInstrumentationOptions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/RemotingInstrumentationOptions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/RemotingInstrumentationOptions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Remoting/Implementation/RemotingInstrumentationEventSource.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Piotr Kiełkowicz <pkielkow@cisco.com>
Hi,
I wanted to revive the amazing work done in #30 with full attribution of work to the original author @mbakalov.
His branch is very out of date, so I decided to fork main and manually move his work over to a new branch on the fork called remoting.
I have kept his work and mainly just done housekeeping renaming and updating to the latest OTEL standards and I have tried to follow the library patterns, but I am sure I am missing some.
Cheers,
Lewis