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

[AzureMonitor] Change drop sampling result to recordOnly #933

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Jan 27, 2023

Fixes #.

Changes

Sampler will now return RecordOnly SamplingResult when the telemetry is sampled instead of Drop. This will result activity object to be created always and populated with all information such as tag, events etc. This is done in order to allow standard
metrics
collection from the generated activities.

The change is needed in order to provide feature parity to current Application Insights SDK customers moving to OpenTelemetry.

Note: This change will have no impact on the overall sampling behavior.

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #933 (f614f34) into main (cad8d76) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head f614f34 differs from pull request most recent head 94bde42. Consider uploading reports for the commit 94bde42 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #933      +/-   ##
==========================================
- Coverage   68.38%   68.38%   -0.01%     
==========================================
  Files         183      183              
  Lines        7026     7025       -1     
==========================================
- Hits         4805     4804       -1     
  Misses       2221     2221              
Impacted Files Coverage Δ
...ensions.AzureMonitor/ApplicationInsightsSampler.cs 96.55% <100.00%> (ø)
...entation.AspNet/TracerProviderBuilderExtensions.cs 100.00% <0.00%> (ø)

@Kielek Kielek added the comp:extensions.azuremonitor Things related to OpenTelemetry.Extensions.AzureMonitor label Jan 30, 2023
@vishweshbankwar vishweshbankwar marked this pull request as ready for review January 31, 2023 00:42
@vishweshbankwar vishweshbankwar requested a review from a team January 31, 2023 00:42
@Kielek
Copy link
Contributor

Kielek commented Jan 31, 2023

Changes looks good, but it will be great to include some explanation why it is needed (maybe first comment in the PR?).

@CodeBlanch
Copy link
Member

@Kielek I was curious too so I chatted with @vishweshbankwar about this. This sampling behavior matches what AppInsights does today so it is about bringing parity for those customers moving to OTel.

@CodeBlanch CodeBlanch merged commit 98989a0 into open-telemetry:main Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:extensions.azuremonitor Things related to OpenTelemetry.Extensions.AzureMonitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants