Skip to content
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

[Instrumentation.ServiceFabricRemoting] initial implementation #2288

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

sablancoleis
Copy link

@sablancoleis sablancoleis commented Nov 3, 2024

Today, Microsoft Service Fabric doesn't support trace context propagation natively. This library fills that gap and allows the trace context and Baggage to be transferred transparently between Services and Actors.

Note: This code targets only Service Fabric Remoting V2, since V1 would require using wrapped messages and it will be deprecated in the near future.

This implementation propagates the ActivityContext and Baggage through SF Remoting using the OpenTelemetry's Propagator infrastructure to inject the data into the message headers from the client and to extract the data from the headers on the receiving listener.

The code also provides extension methods to configure the whole infrastructure with one line of code in Program.cs:
tracerProviderBuilder.AddServiceFabricRemotingInstrumentation()

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@sablancoleis sablancoleis requested a review from a team as a code owner November 3, 2024 16:11
Copy link

linux-foundation-easycla bot commented Nov 3, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the infra Infra work - CI/CD, code coverage, linters label Nov 3, 2024
@rajkumar-rangaraj
Copy link
Contributor

@sablanc-msft Could you please sign the Easy CLA, it's a requirement to review/accept PRs.

@rajkumar-rangaraj
Copy link
Contributor

@sablanc-msft You need to update the https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/.github/component_owners.yml file with owners. It is recommended to have a minimum of 2 owners. Once the component owner review is complete, approvers/maintainers will review.

Also, it would be nice if you provide a clear PR description of what you are trying to solve by adding this project to the repo.

@Kielek
Copy link
Contributor

Kielek commented Nov 14, 2024

One more comment, do you see any possibility to include native-instrumentation directly in ServiceFabric? Whole Azure SDK is following this pattern, and it is the best option we can recommend.

@sablancoleis
Copy link
Author

One more comment, do you see any possibility to include native-instrumentation directly in ServiceFabric? Whole Azure SDK is following this pattern, and it is the best option we can recommend.

That would be ideal. Hopefully the Service Fabric team can include native instrumentation in future releases. This library is meant to fill the gap of context propagation in the meantime.

@lmolkova
Copy link
Contributor

@sablancoleis have you tried reaching out to the ServiceFabric team and proposing to add instrumentation there?

It seems they were considering adding instrumentation at some point and might be open to someone contributing it - microsoft/service-fabric-services-and-actors-dotnet#294

@sablancoleis
Copy link
Author

@sablancoleis have you tried reaching out to the ServiceFabric team and proposing to add instrumentation there?

It seems they were considering adding instrumentation at some point and might be open to someone contributing it - microsoft/service-fabric-services-and-actors-dotnet#294

@lmolkova , Yes, I contacted them a month and a half ago and multiple times during the past few weeks, but I haven't been able to get a response yet. I will add you to the email thread.

I hope the SF Team integrates context propagation in a future release, but it will take months and even after that, it will take months for that release to be upgraded in the applications trying to use OpenTelemetry.
This library allows my Team, other teams in Microsoft and external customers using Service Fabric to integrate OpenTelemetry without having to upgrade the Service Fabric SDK, so I think it still adds value while we wait for the native integration.
My team is already using this code in production, but I would like to get it reviewed by the OpenTelemetry community and the Service Fabric team.

…ace the individual client factories for Services and Actors.
@sablancoleis
Copy link
Author

One more comment, do you see any possibility to include native-instrumentation directly in ServiceFabric? Whole Azure SDK is following this pattern, and it is the best option we can recommend.

@Kielek, I got confirmation from the Service Fabric Team that they don't have plans to integrate OpenTelemetry in the near future.

Copy link
Contributor

@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.

Just some technical comments.
I do not know anything about ServiceFabric. In this context it will be great to find someone to review.

That is sad, the ServiceFabric does not want to implement such stuff. Maybe you can consider contributing there? I am not against including this instrumentation library here, but it will be great to find the agreement also internally within MS teams.

@@ -57,6 +57,8 @@ components:
src/OpenTelemetry.Instrumentation.Runtime/:
- twenzel
- xiang17
src/OpenTelemetry.Instrumentation.ServiceFabricRemoting/:
- sablancoleis
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance to have second volunteer to help with the maintenance?

@@ -47,6 +47,7 @@ jobs:
instrumentation-process: ['*/OpenTelemetry.Instrumentation.Process*/**', 'examples/process-instrumentation/**', '!**/*.md']
instrumentation-quartz: ['*/OpenTelemetry.Instrumentation.Quartz*/**', '!**/*.md']
instrumentation-runtime: ['*/OpenTelemetry.Instrumentation.Runtime*/**', 'examples/runtime-instrumentation/**', '!**/*.md']
instrumentation-servicefabricremoting: ['*/OpenTelemetry.Instrumentation.ServiceFabricRemoting*/**', 'examples/servicefabricremoting-instrumentation/**', '!**/*.md']
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see examples so this can be removed)

Suggested change
instrumentation-servicefabricremoting: ['*/OpenTelemetry.Instrumentation.ServiceFabricRemoting*/**', 'examples/servicefabricremoting-instrumentation/**', '!**/*.md']
instrumentation-servicefabricremoting: ['*/OpenTelemetry.Instrumentation.ServiceFabricRemoting*/**', '!**/*.md']

Comment on lines +21 to +22
<PackageReference Include="OpenTelemetry.Api" Version="$(OpenTelemetryCoreLatestVersion)" />
<PackageReference Include="OpenTelemetry.Api.ProviderBuilderExtensions" Version="$(OpenTelemetryCoreLatestVersion)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

OpenTelemetry.Api is transitive dependency. You can simplify here:

Suggested change
<PackageReference Include="OpenTelemetry.Api" Version="$(OpenTelemetryCoreLatestVersion)" />
<PackageReference Include="OpenTelemetry.Api.ProviderBuilderExtensions" Version="$(OpenTelemetryCoreLatestVersion)" />
<PackageReference Include="OpenTelemetry.Api.ProviderBuilderExtensions" Version="$(OpenTelemetryCoreLatestVersion)" />

Comment on lines +3 to +6
| Status | |
| ------------- |-----------|
| Stability | |
| Code Owners | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update

Comment on lines +18 to +19
An example project is available in the
[examples//serviceFabricRemoting](../../examples/serviceFabricRemoting/) folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not true. I am not sure if you need an example. Or you can have cool enough README here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Infra work - CI/CD, code coverage, linters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants