Perform sampling as explained in the specification. #839

hdost · 2022-07-14T19:49:46Z

Sampling should always defer to the existing sampler. The logic before
this patch reflected something similar to what is described in the
ParentBased sampler.

Relates #800

codecov · 2022-07-14T20:14:03Z

Codecov Report

Base: 66.6% // Head: 66.5% // Decreases project coverage by -0.0% ⚠️

Coverage data is based on head (c91f56d) compared to base (ffe1fbb).
Patch has no changes to coverable lines.

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

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #839     +/-   ##
=======================================
- Coverage   66.6%   66.5%   -0.1%     
=======================================
  Files        113     113             
  Lines       8798    8712     -86     
=======================================
- Hits        5861    5799     -62     
+ Misses      2937    2913     -24

Impacted Files	Coverage Δ
opentelemetry-sdk/src/trace/tracer.rs	`92.5% <ø> (+1.5%)`	⬆️
opentelemetry-jaeger/src/exporter/config/agent.rs	`58.0% <0.0%> (-2.6%)`	⬇️
...emetry-jaeger/src/exporter/config/collector/mod.rs	`56.5% <0.0%> (-1.8%)`	⬇️
opentelemetry-sdk/src/testing/trace.rs	`69.3% <0.0%> (-1.2%)`	⬇️
opentelemetry-api/src/trace/span_context.rs	`87.2% <0.0%> (-1.1%)`	⬇️
opentelemetry-jaeger/src/lib.rs	`93.0% <0.0%> (-0.4%)`	⬇️
opentelemetry-aws/src/lib.rs	`89.8% <0.0%> (ø)`
opentelemetry-jaeger/src/exporter/config/mod.rs	`75.5% <0.0%> (ø)`
...aeger/src/exporter/config/collector/http_client.rs	`22.5% <0.0%> (ø)`
...elemetry-sdk/src/metrics/aggregators/last_value.rs	`46.2% <0.0%> (+0.8%)`	⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

opentelemetry-api/src/trace/tracer.rs

jtescher · 2022-08-14T19:02:20Z

@hdost want some help making the last changes? Think this is the only blocker to releasing v0.18

hdost · 2022-08-16T15:58:42Z

@hdost want some help making the last changes? Think this is the only blocker to releasing v0.18

I can push out a change tomorrow 👍🏼

hdost · 2022-08-17T17:29:54Z

Sorry again for the delay on this. All should be good now.

hdost · 2022-08-17T18:23:27Z

Sorry again for the delay on this. All should be good now.

Unsure why i have an issue with msrv...

djc · 2022-08-17T20:06:07Z

Because there was a new release of the h2 library recently, and it requires 1.56. I don't think our 1.49 is tenable anymore, we should just bump to 1.56 like the rest of the ecosystem.

hdost · 2022-08-18T11:42:41Z

Because there was a new release of the h2 library recently, and it requires 1.56. I don't think our 1.49 is tenable anymore, we should just bump to 1.56 like the rest of the ecosystem.

Indeed 😞
@jtescher @TommyCpp what are your thoughts?

jtescher · 2022-08-18T16:28:50Z

@jtescher @TommyCpp what are your thoughts?

I'm in favor of going to 1.56 and 2021 edition 👍

TommyCpp · 2022-08-18T16:31:29Z

Agreed. I think tracing also have a PR to bump the MSRV too

hdost · 2022-08-22T09:27:28Z

Agreed. I think tracing also have a PR to bump the MSRV too

It appears so though it's been open for a month i feel they probably will. , should we open a separate PR for MSRV increase?

djc · 2022-08-22T09:28:52Z

See #866.

hdost · 2022-09-06T13:19:38Z

😢

error: package `async-global-executor v2.3.0` cannot be built because it requires rustc 1.59 or newer, while the currently active rustc version is 1.56.1

jtescher · 2022-09-06T17:53:36Z

@hdost if you merge main you'll pick up the script changes to get MSRV passing again

* Sampling should always defer to the existing sampler. The logic before this patch reflected something similar to what is described in the ParentBased sampler. * Maintaining pre-sample to allow for `tracing-opentelemetry` to continue working Relates open-telemetry#800 Signed-off-by: Harold Dost <h.dost@criteo.com>

hdost · 2022-09-09T09:58:15Z

Finally! 👍🏼 Things are passing

## Motivation Support the latest OpenTelemetry specification. ## Solution Update `opentelemetry` to the latest `0.18.x` release. Breaking changes in the metrics spec have removed value recorders and added histograms so the metrics layer's `value.` prefix has been changed to `histogram.` and behaves accordingly. Additionally the `PushController` configuration for the metrics layer has been simplified to accept a `BasicController` that can act in either push or pull modes. Finally trace sampling in the sdk's `PreSampledTracer` impl has been updated to match the sampling logic in open-telemetry/opentelemetry-rust#839.

jtescher · 2022-09-13T03:52:28Z

@hdost if you want to become an approver (requirements) I'm happy to sponsor you, just have open an issue here to get added.

TommyCpp · 2022-09-13T04:43:15Z

@hdost if you want to become an approver (requirements) I'm happy to sponsor you, just have open an issue here to get added.

+1 if you need another sponsor I can help. Again thanks for all the work on the sampling side!

## Motivation Support the latest OpenTelemetry specification. ## Solution Update `opentelemetry` to the latest `0.18.x` release. Breaking changes in the metrics spec have removed value recorders and added histograms so the metrics layer's `value.` prefix has been changed to `histogram.` and behaves accordingly. Additionally the `PushController` configuration for the metrics layer has been simplified to accept a `BasicController` that can act in either push or pull modes. Finally trace sampling in the sdk's `PreSampledTracer` impl has been updated to match the sampling logic in open-telemetry/opentelemetry-rust#839. * Update MSRV to 1.56 * Update examples * Fix async-trait dep

## Motivation Support the latest OpenTelemetry specification. ## Solution Update `opentelemetry` to the latest `0.18.x` release. Breaking changes in the metrics spec have removed value recorders and added histograms so the metrics layer's `value.` prefix has been changed to `histogram.` and behaves accordingly. Additionally the `PushController` configuration for the metrics layer has been simplified to accept a `BasicController` that can act in either push or pull modes. Finally trace sampling in the sdk's `PreSampledTracer` impl has been updated to match the sampling logic in open-telemetry/opentelemetry-rust#839. * Update MSRV to 1.56 * Update examples * Fix async-trait dep # Conflicts: # .github/workflows/CI.yml # tracing-opentelemetry/Cargo.toml # tracing-opentelemetry/src/layer.rs # tracing-opentelemetry/src/metrics.rs # tracing-opentelemetry/tests/metrics_publishing.rs

## Motivation Support the latest OpenTelemetry specification. ## Solution Update `opentelemetry` to the latest `0.18.x` release. Breaking changes in the metrics spec have removed value recorders and added histograms so the metrics layer's `value.` prefix has been changed to `histogram.` and behaves accordingly. Additionally the `PushController` configuration for the metrics layer has been simplified to accept a `BasicController` that can act in either push or pull modes. Finally trace sampling in the sdk's `PreSampledTracer` impl has been updated to match the sampling logic in open-telemetry/opentelemetry-rust#839. * Update MSRV to 1.56 * Update examples * Fix async-trait dep * Update msrv action Co-authored-by: Eliza Weisman <eliza@buoyant.io>

## Motivation Support the latest OpenTelemetry specification. ## Solution Update `opentelemetry` to the latest `0.18.x` release. Breaking changes in the metrics spec have removed value recorders and added histograms so the metrics layer's `value.` prefix has been changed to `histogram.` and behaves accordingly. Additionally the `PushController` configuration for the metrics layer has been simplified to accept a `BasicController` that can act in either push or pull modes. Finally trace sampling in the sdk's `PreSampledTracer` impl has been updated to match the sampling logic in open-telemetry/opentelemetry-rust#839. * Update MSRV to 1.56 * Update examples * Fix async-trait dep

## Motivation Support the latest OpenTelemetry specification. ## Solution Update `opentelemetry` to the latest `0.18.x` release. Breaking changes in the metrics spec have removed value recorders and added histograms so the metrics layer's `value.` prefix has been changed to `histogram.` and behaves accordingly. Additionally the `PushController` configuration for the metrics layer has been simplified to accept a `BasicController` that can act in either push or pull modes. Finally trace sampling in the sdk's `PreSampledTracer` impl has been updated to match the sampling logic in open-telemetry/opentelemetry-rust#839. * Update MSRV to 1.56 * Update examples * Fix async-trait dep * Update msrv action Co-authored-by: Eliza Weisman <eliza@buoyant.io>

hdost requested a review from a team July 14, 2022 19:49

hdost force-pushed the feat/using-samplers-correctly branch from 8971da0 to d3a777a Compare July 14, 2022 19:53

jtescher reviewed Jul 14, 2022

View reviewed changes

opentelemetry-api/src/trace/tracer.rs Outdated Show resolved Hide resolved

hdost mentioned this pull request Jul 17, 2022

Sampling Trouble #800

Closed

hdost force-pushed the feat/using-samplers-correctly branch from d3a777a to b395c8b Compare August 17, 2022 17:29

hdost force-pushed the feat/using-samplers-correctly branch from b395c8b to 91a25da Compare August 17, 2022 17:36

fasterthanlime mentioned this pull request Aug 30, 2022

Exporters should not be sent "is_sampled=false" spans #870

Closed

jtescher mentioned this pull request Sep 5, 2022

Publish crate for opentelemetry-proto #860

Closed

hdost force-pushed the feat/using-samplers-correctly branch from c91f56d to 0b207af Compare September 6, 2022 13:14

hdost force-pushed the feat/using-samplers-correctly branch from 0b207af to a0f8e04 Compare September 8, 2022 13:18

jtescher approved these changes Sep 9, 2022

View reviewed changes

jtescher merged commit 6c1e487 into open-telemetry:main Sep 9, 2022

hdost deleted the feat/using-samplers-correctly branch September 9, 2022 22:44

jtescher mentioned this pull request Sep 13, 2022

opentelemetry: Update otel to 0.18.0 tokio-rs/tracing#2303

Merged

hdost mentioned this pull request Sep 13, 2022

REQUEST: New membership for hdost open-telemetry/community#1179

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform sampling as explained in the specification. #839

Perform sampling as explained in the specification. #839

hdost commented Jul 14, 2022

codecov bot commented Jul 14, 2022 •

edited

Loading

jtescher commented Aug 14, 2022

hdost commented Aug 16, 2022

hdost commented Aug 17, 2022

hdost commented Aug 17, 2022

djc commented Aug 17, 2022

hdost commented Aug 18, 2022

jtescher commented Aug 18, 2022

TommyCpp commented Aug 18, 2022

hdost commented Aug 22, 2022

djc commented Aug 22, 2022

hdost commented Sep 6, 2022

jtescher commented Sep 6, 2022

hdost commented Sep 9, 2022

jtescher commented Sep 13, 2022

TommyCpp commented Sep 13, 2022 •

edited

Loading

Perform sampling as explained in the specification. #839

Perform sampling as explained in the specification. #839

Conversation

hdost commented Jul 14, 2022

codecov bot commented Jul 14, 2022 • edited Loading

Codecov Report

jtescher commented Aug 14, 2022

hdost commented Aug 16, 2022

hdost commented Aug 17, 2022

hdost commented Aug 17, 2022

djc commented Aug 17, 2022

hdost commented Aug 18, 2022

jtescher commented Aug 18, 2022

TommyCpp commented Aug 18, 2022

hdost commented Aug 22, 2022

djc commented Aug 22, 2022

hdost commented Sep 6, 2022

jtescher commented Sep 6, 2022

hdost commented Sep 9, 2022

jtescher commented Sep 13, 2022

TommyCpp commented Sep 13, 2022 • edited Loading

codecov bot commented Jul 14, 2022 •

edited

Loading

TommyCpp commented Sep 13, 2022 •

edited

Loading