Skip to content

Add WCF support for IRequestSessionChannel and IDuplexChannel#1374

Merged
Kielek merged 8 commits into
open-telemetry:mainfrom
repl-chris:RequestSessionChannelSupport
Oct 5, 2023
Merged

Add WCF support for IRequestSessionChannel and IDuplexChannel#1374
Kielek merged 8 commits into
open-telemetry:mainfrom
repl-chris:RequestSessionChannelSupport

Conversation

@repl-chris
Copy link
Copy Markdown
Contributor

@repl-chris repl-chris commented Sep 25, 2023

Fixes #1369

Changes

The provided configuration from the issue uses an IRequestSessionChannel shape, which the instrumentation did not yet support. This PR adds support for IRequestSessionChannel. I've also enabled support for IDuplexChannel shaped channels, since the implementation actually already supported it as part of the IDuplexSessionChannel implementation.

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

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

@repl-chris repl-chris requested a review from a team September 25, 2023 15:32
@github-actions github-actions Bot requested a review from CodeBlanch September 25, 2023 15:32
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 25, 2023

Codecov Report

Merging #1374 (3a05065) into main (71655ce) will increase coverage by 0.16%.
Report is 18 commits behind head on main.
The diff coverage is 76.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1374      +/-   ##
==========================================
+ Coverage   73.91%   74.07%   +0.16%     
==========================================
  Files         267      258       -9     
  Lines        9615     9551      -64     
==========================================
- Hits         7107     7075      -32     
+ Misses       2508     2476      -32     
Flag Coverage Δ
unittests-Instrumentation.Wcf 79.01% <82.27%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 95.10% <100.00%> (+1.76%) ⬆️
...searchClient/ElasticsearchClientInstrumentation.cs 100.00% <100.00%> (ø)
...ityFrameworkCore/EntityFrameworkInstrumentation.cs 100.00% <100.00%> (ø)
...ation/EntityFrameworkInstrumentationEventSource.cs 12.00% <100.00%> (ø)
...ation.Owin/Implementation/DiagnosticsMiddleware.cs 89.42% <100.00%> (-2.44%) ⬇️
....Owin/Implementation/OwinInstrumentationMetrics.cs 100.00% <100.00%> (ø)
...inInstrumentationMeterProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...Instrumentation.Quartz/QuartzJobInstrumentation.cs 83.33% <100.00%> (+1.51%) ⬆️
...entation.Wcf/Implementation/InstrumentedChannel.cs 100.00% <100.00%> (ø)
...f/Implementation/InstrumentedChannelFactoryBase.cs 100.00% <100.00%> (ø)
... and 16 more

... and 33 files with indirect coverage changes

@repl-chris repl-chris changed the title Add support for IRequestSessionChannel and IDuplexChannel Add WCF support for IRequestSessionChannel and IDuplexChannel Sep 25, 2023
@Kielek Kielek added the comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf label Sep 26, 2023
}

protected ICommunicationObject Inner { get; }
protected T Inner { get; }
Copy link
Copy Markdown
Member

@CodeBlanch CodeBlanch Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@repl-chris I just pushed a little bit of refactoring so that we can expose a strongly-typed "Inner" on the base class. That gets rid of the need for [private|protected] new [ChannelType] Inner { get => ([ChannelType])base.Inner; } we had been using in all the derived classes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice....looks good 👍

@CodeBlanch
Copy link
Copy Markdown
Member

@repl-chris This looks good to me but before we can merge we need to update the CHANGELOG and perhaps the README with the new supported matrix?

@github-actions github-actions Bot requested a review from CodeBlanch October 5, 2023 17:30
@repl-chris
Copy link
Copy Markdown
Contributor Author

repl-chris commented Oct 5, 2023

@repl-chris This looks good to me but before we can merge we need to update the CHANGELOG and perhaps the README with the new supported matrix?

@CodeBlanch done 👍

Copy link
Copy Markdown
Member

@Kielek Kielek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chnagelog and README LGTM. Approving and merging based on previous @CodeBlanch comment

@Kielek Kielek merged commit 2529398 into open-telemetry:main Oct 5, 2023
@repl-chris repl-chris deleted the RequestSessionChannelSupport branch October 5, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NetTcpBinding, Streamed transfer and TransportWithMessageCredential incompatible with TelemetryEndpointBehavior

3 participants