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.AspNetCore] Fix issue causing Activity is not exported when used with custom propagator/sampler. #4637

Merged
merged 9 commits into from
Jul 10, 2023
Merged

[Instrumentation.AspNetCore] Fix issue causing Activity is not exported when used with custom propagator/sampler. #4637

merged 9 commits into from
Jul 10, 2023

Conversation

TimothyMothra
Copy link
Contributor

Fixes #4626

When a custom propagator is used, the Instrumentation.AspNetCore library will create a new activity that is identified by a specific tag ("IsCreatedByInstrumentation").
For performance, only the first tag was inspected.

However, if a custom sampler is also used and adds tags that will affect the ordering and the instrumentation library will not identify the activity.
This causes activities to be dropped.

Changes

  • Added new extension method ActivityHelperExtensions.TryGetTagValue()
  • Fix HttpInListener to inspect all tags, not just first.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (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)

@TimothyMothra TimothyMothra requested a review from a team July 6, 2023 00:51
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #4637 (5cae461) into main (66a6062) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4637      +/-   ##
==========================================
- Coverage   84.98%   84.96%   -0.03%     
==========================================
  Files         314      314              
  Lines       12683    12685       +2     
==========================================
- Hits        10779    10778       -1     
- Misses       1904     1907       +3     
Impacted Files Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs 90.47% <100.00%> (+0.10%) ⬆️

... and 4 files with indirect coverage changes

TimothyMothra and others added 2 commits July 7, 2023 10:09
@@ -95,6 +95,24 @@ public static object GetTagValue(this Activity activity, string tagName)
return null;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool TryGetTagValue(this Activity activity, string tagName, out object tagValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How strongly do you feel about using the existing method?

I think the TryGet is cleaner and saves an unnecessary assertion (if returnValue != null).

Copy link
Member

Choose a reason for hiding this comment

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

I think the min bar is:

  1. don't duplicate code (e.g. if we want to add this method, at least the existing one GetTagValue should be updated so it'll leverage the TryGetTagValue).
  2. keep it consistent - examine the existing usage and make proper update.

Copy link
Contributor

@utpilla utpilla Jul 7, 2023

Choose a reason for hiding this comment

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

saves an unnecessary assertion if (returnValue != null)

Even with the new method you have to check if (returnValue == true).

I would be in favor of reusing the existing method in this case and avoid having to introduce more code (and related tests) just to achieve a boolean check instead of a null check.

Copy link
Member

Choose a reason for hiding this comment

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

I would be in favor of reusing the existing method in this case and avoid having to introduce more code (and related tests) just to achieve a boolean check instead of a null check.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@utpilla utpilla merged commit b23b146 into open-telemetry:main Jul 10, 2023
@TimothyMothra TimothyMothra deleted the 4626_fixCustomPropagatorSampler branch July 10, 2023 23:31
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.

[Instrumentation.AspNetCore] [.NET7.0] Activity is not exported when used with custom propagator/sampler
4 participants