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

Add api-version support #4532

Closed
wants to merge 1 commit into from

Conversation

dhabierre
Copy link

When using a versioned API with Microsoft.AspNetCore.Mvc.Versioning nuget package, the {version:apiVersion} is not replaced with the real controller version.

Discussion issue #2967 & #4525

Changes

  • Update OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs in order to replace {version:apiVersion} url segment with the real controller version.
  • Add ApiVersioning support & ApiVersioningControllers in TestApp.AspNetCore project

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes

This is my first PR here, hope all is fine :)
Any feedback about my proposal is welcome.

@dhabierre dhabierre requested a review from a team May 31, 2023 21:24
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 31, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@alanwest
Copy link
Member

alanwest commented Jun 1, 2023

@dhabierre Thanks for the contribution! Including API version information in the Activity name (and/or the http.route attribute) sounds like it could be a really good idea. However, we're going to hold off on accepting this PR at the moment.

There is a strong push at the spec level going on at the moment to stabilize the HTTP semantic conventions and we're actively working to align our instrumentation to the conventions. Routing is complex as there are many scenarios to consider handling (API versioning being a potential scenario), so naming the Activity and including the route information is one of the trickier parts to hash out before we can release stable ASP.NET Core instrumentation.

@vishweshbankwar will be working with the ASP.NET Core team to sort out the optimal way to retrieve the route information we include in the Activity. We'll add API version to the list of things we're considering. Please bear with us as we work through this.

@vishweshbankwar suggested a potential solution in the interim could be to use Enrich to modify the Activity as you need it.

@dhabierre
Copy link
Author

Hi @alanwest and thanks for your post.
Hope that versionned APIs will be supported soon :)

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.

3 participants