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.AspNet] pass http.request.method to sampler #2023

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

Conversation

roycald245
Copy link

@roycald245 roycald245 commented Aug 26, 2024

Fixes #57287

Changes

Added http.request.method to tags given to sampler

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)

@github-actions github-actions bot added the comp:instrumentation.aspnet.telemetryhttpmodule Things related to OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule label Aug 26, 2024
@github-actions github-actions bot added comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet documentation Improvements or additions to documentation labels Aug 26, 2024
@roycald245 roycald245 marked this pull request as ready for review August 26, 2024 09:56
@roycald245 roycald245 requested a review from a team August 26, 2024 09:56
Co-authored-by: Cijo Thomas <[email protected]>
@Kielek Kielek changed the title Feat/add http method [Instrumentation.AspNet] pass http.request.method to sampler Aug 27, 2024
{
tags = cachedTagsStorage ??= new KeyValuePair<string, object?>[1];
string? path = context.Request?.Unvalidated?.Path;
string? method = context.Request?.HttpMethod;
Copy link
Member

@vishweshbankwar vishweshbankwar Aug 29, 2024

Choose a reason for hiding this comment

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

I think we need to set the normalized value as we do here

If the method is normalized to _OTHER then should we also include http.request.method_original, that part is not very clear in spec

Copy link
Author

Choose a reason for hiding this comment

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

fixed 34e7fa3

Copy link
Author

Choose a reason for hiding this comment

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

I think setting "HTTP" like the example you sent is the way to go. Keeping the convention.

@birojnayak
Copy link
Contributor

@cijothomas @Kielek this would be really useful for us.... can we have it available sooner

@cijothomas cijothomas requested a review from a team as a code owner September 19, 2024 16:00
@Kielek
Copy link
Contributor

Kielek commented Sep 24, 2024

@roycald245, one test is failing.

@roycald245
Copy link
Author

I had some distractions recently. Will fix as soon as I'm able

Copy link
Contributor

github-actions bot commented Oct 3, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 3, 2024
- handle http method attributes as in RequestDataHelper.SetHttpMethodTag
- fix cashing tags storage
- extend tests
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.32%. Comparing base (71655ce) to head (5c1b72e).
Report is 535 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2023      +/-   ##
==========================================
+ Coverage   73.91%   77.32%   +3.40%     
==========================================
  Files         267       15     -252     
  Lines        9615      366    -9249     
==========================================
- Hits         7107      283    -6824     
+ Misses       2508       83    -2425     
Flag Coverage Δ
unittests-Instrumentation.AspNet 77.32% <100.00%> (?)

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

Files with missing lines Coverage Δ
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs 87.83% <100.00%> (+2.59%) ⬆️

... and 267 files with indirect coverage changes

@Kielek Kielek removed the Stale label Oct 3, 2024
@Kielek
Copy link
Contributor

Kielek commented Oct 3, 2024

@roycald245, I have made some direct changes to

  • handle http method attributes as in RequestDataHelper.SetHttpMethodTag
  • fix cashing tags storage
  • extend tests

Please double check if you are fine with this solution.

@cijothomas, could you please review?

Copy link
Contributor

@ysolomchenko ysolomchenko left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aspnet.telemetryhttpmodule Things related to OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add http method tag for activity creation
6 participants