Skip to content

Conversation

@martincostello
Copy link
Member

  • Add new overload to LabelsWrapper to support passing through state to avoid allocating closures.
  • Update the application in the integration tests to also target .NET 8 and use more modern C# features.
  • Update integration tests to use Python 3.12.
  • Fix some code analysis warnings.

Cherry-picked from #118.

Add an overload of `LabelsWrapper.Do()` that allows state to be passed to the invocation to avoid allocating a closure when state needs to captured from the caller.
- Make readonly field `readonly`.
- Make private class `sealed`.
Update load generator to use Python 3.12.
- Update the ASP.NET Core sample application to use modern Minimal API approach and latest C# features.
- Update the ASP.NET Core sample application to support multi-targeting.
@martincostello martincostello marked this pull request as ready for review November 3, 2025 14:38
Copilot AI review requested due to automatic review settings November 3, 2025 14:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the integration test application to support multi-targeting, improves code quality, and adds new tracing integrations. The changes include updating the project to target both .NET 8.0 and .NET 6.0, refactoring the application to use dependency injection, and enhancing the Pyroscope library with a new generic overload for label wrapping.

  • Added support for multi-targeting .NET 8.0 and .NET 6.0
  • Refactored integration test code to use modern C# patterns and dependency injection
  • Extended LabelsWrapper with a generic overload to support stateful lambda callbacks

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
itest.Dockerfile Updated sed command to match TargetFrameworks (plural) in the Dockerfile build step
Pyroscope/Pyroscope/LabelsWrapper.cs Added generic Do<T> overload and extracted common cleanup logic to ResetLabels method
Pyroscope/Pyroscope.sln Added three new projects to the solution (OpenTelemetry, OpenTracing, and Rideshare integration test)
Pyroscope/Pyroscope.OpenTracing/PyroscopeSpanBuilder.cs Changed _parent field to readonly and made PyroscopeScope class private sealed
IntegrationTest/ScooterService.cs Removed unused using System.Diagnostics, made DoSomeOtherWork static, and improved numeric literal readability
IntegrationTest/Rideshare.csproj Changed from single target framework to multi-targeting, enabled implicit usings and latest language version
IntegrationTest/Properties/launchSettings.json Added new launch profile for local development
IntegrationTest/Program.cs Refactored to use modern top-level statements and dependency injection instead of static service instances
IntegrationTest/OrderService.cs Refactored to use new generic LabelsWrapper.Do overload with static lambdas, removed unused variable
IntegrationTest/Dockerfile.load-generator Updated Python version from 3.9 to 3.12
IntegrationTest/CarService.cs Converted method to expression-bodied member
IntegrationTest/BikeService.cs Converted method to expression-bodied member

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant