Avoid overriding configuration of tracer provider #1633

ijsong · 2021-03-03T15:27:53Z

This change adds WithDefaultSampler and WithSpanLimits to the tracer provider and removed WithConfig from it.

Before this change, WithConfig is the only way to set sampler or limits of a span. However, it is prone to misuse, since WithConfig can override tracing configurations that are configured by WithResource or
WithIDGenerator. Thus to fix this, it adds new functional options -WithDefaultSampler and WithSpanLimits and removes WithConfig.

Resolves #1631.

codecov · 2021-03-03T15:31:02Z

Codecov Report

Merging #1633 (e942912) into main (2b4d5ac) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main   #1633   +/-   ##
=====================================
  Coverage   77.1%   77.2%           
=====================================
  Files        128     128           
  Lines       6735    6742    +7     
=====================================
+ Hits        5196    5205    +9     
+ Misses      1288    1286    -2     
  Partials     251     251

Impacted Files	Coverage Δ
exporters/otlp/internal/otlptest/otlptest.go	`74.5% <100.0%> (ø)`
exporters/trace/jaeger/jaeger.go	`93.6% <100.0%> (+0.1%)`	⬆️
sdk/trace/provider.go	`83.0% <100.0%> (+0.2%)`	⬆️
exporters/otlp/otlpgrpc/connection.go	`88.7% <0.0%> (+1.8%)`	⬆️

MrAlias

Looks good overall, minor documentation fixes needed.

sdk/trace/provider.go

Aneurysm9

This looks good to me, though as @MrAlias mentioned in Slack it would be good if we went one step further and avoided exporting the TracerProvider.ApplyConfig method and related Config struct. It looks like they're only used in a couple places that should be able to be rewritten using the new options.

MrAlias · 2021-03-03T19:59:41Z

This looks good to me, though as @MrAlias mentioned in Slack it would be good if we went one step further and avoided exporting the TracerProvider.ApplyConfig method and related Config struct. It looks like they're only used in a couple places that should be able to be rewritten using the new options.

Tracking this topic here so the scope of this PR isn't overloaded.

ijsong · 2021-03-03T23:42:48Z

It seems to be unrelated. Waiting time for reconnection maybe not enough sometimes.

--- FAIL: TestNewExporter_collectorConnectionDiesThenReconnects (0.25s)
    otlp_integration_test.go:248: 
        	Error Trace:	otlp_integration_test.go:248
        	Error:      	Received unexpected error:
        	            	no client
        	Test:       	TestNewExporter_collectorConnectionDiesThenReconnects
FAIL

MrAlias · 2021-03-08T18:13:43Z

@ijsong it looks like WithConfig was added in a few more places upstream. Let me know if you want help resolving these conflicts. Otherwise, I think this looks good to merge once they are resolved.

This change adds `WithDefaultSampler` and `WithSpanLimits` to the tracer provider and removed `WithConfig` from it. Before this change, `WithConfig` is the only way to set sampler or limits of a span. However, it is prone to misuse, since `WithConfig` can override tracing configurations that are configured by `WithResource` or `WithIDGenerator`. Thus to fix this, it adds new functional options - `WithDefaultSampler` and `WithSpanLimits` and removes `WithConfig`. Resolves open-telemetry#1631.

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

XSAM

We might need to write a tool to prevent changes to the released changelog.

XSAM · 2021-03-09T02:42:29Z

CHANGELOG.md

@@ -56,6 +56,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   | Windows | 1.14       | amd64        |
   | Windows | 1.15       | 386          |
   | Windows | 1.14       | 386          |
+- Added `WithDefaultSampler` and `WithSpanLimits` to tracer provider. (#1633)


It seems it is the wrong place to put this changelog.

Sorry to not have checked carefully.

That is fine. This should be handled by an automatic tool, which lowers everyone's mental burden. ;)

ijsong requested review from Aneurysm9, dashpole, evantorrie, jmacd, lizthegrey, MrAlias and XSAM as code owners March 3, 2021 15:27

MrAlias requested changes Mar 3, 2021

View reviewed changes

sdk/trace/provider.go Outdated Show resolved Hide resolved

sdk/trace/provider.go Outdated Show resolved Hide resolved

Aneurysm9 reviewed Mar 3, 2021

View reviewed changes

MrAlias mentioned this pull request Mar 3, 2021

Remove TracerProvider.ApplyConfig and Config #1636

Closed

MrAlias approved these changes Mar 3, 2021

View reviewed changes

ijsong force-pushed the tracer-provider-config branch from 62a661b to 665fcb0 Compare March 3, 2021 22:29

MrAlias added this to the RC1 milestone Mar 4, 2021

MrAlias added area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package labels Mar 4, 2021

Aneurysm9 approved these changes Mar 5, 2021

View reviewed changes

ijsong and others added 4 commits March 9, 2021 06:16

Update sdk/trace/provider.go

a2deff6

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

Update sdk/trace/provider.go

0e2f7a9

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

rebase and remove WithConfig

4117ec1

ijsong force-pushed the tracer-provider-config branch from 18ec5ad to 4117ec1 Compare March 8, 2021 21:37

MrAlias merged commit d75e268 into open-telemetry:main Mar 8, 2021

XSAM reviewed Mar 9, 2021

View reviewed changes

XSAM mentioned this pull request Mar 9, 2021

Fix CHANGELOG #1680

Merged

MrAlias mentioned this pull request Mar 18, 2021

Release v0.19.0 #1710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid overriding configuration of tracer provider #1633

Avoid overriding configuration of tracer provider #1633

ijsong commented Mar 3, 2021

codecov bot commented Mar 3, 2021 •

edited

Loading

MrAlias left a comment

Aneurysm9 left a comment

MrAlias commented Mar 3, 2021 •

edited

Loading

ijsong commented Mar 3, 2021

MrAlias commented Mar 8, 2021

XSAM left a comment

XSAM Mar 9, 2021

ijsong Mar 9, 2021

XSAM Mar 9, 2021

Avoid overriding configuration of tracer provider #1633

Avoid overriding configuration of tracer provider #1633

Conversation

ijsong commented Mar 3, 2021

codecov bot commented Mar 3, 2021 • edited Loading

Codecov Report

MrAlias left a comment

Choose a reason for hiding this comment

Aneurysm9 left a comment

Choose a reason for hiding this comment

MrAlias commented Mar 3, 2021 • edited Loading

ijsong commented Mar 3, 2021

MrAlias commented Mar 8, 2021

XSAM left a comment

Choose a reason for hiding this comment

XSAM Mar 9, 2021

Choose a reason for hiding this comment

ijsong Mar 9, 2021

Choose a reason for hiding this comment

XSAM Mar 9, 2021

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2021 •

edited

Loading

MrAlias commented Mar 3, 2021 •

edited

Loading