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

Update API configs. #1921

Merged
merged 9 commits into from
May 27, 2021
Merged

Conversation

MadVikingGod
Copy link
Contributor

@MadVikingGod MadVikingGod commented May 14, 2021

This change will update the configs in the base API, go.opentelemetry.io/otel/trace and go.opentelemetry.io/otel/metric`, to reflect our contributing documentation.

Because these configs are consumed by the SDK, and any other alternative implementation, they can not be private. This also means that the members of these configs will need some accessor.

This will finish the last items of #1882, closes #536

In addition, this reorganizes spanOptions to prevent ignored options from being accepted in any steps. For example Span.End() no longer accepts attributes or links. Closes #1878

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #1921 (5ee455a) into main (270cc60) will increase coverage by 0.1%.
The diff coverage is 85.7%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1921     +/-   ##
=======================================
+ Coverage   79.5%   79.6%   +0.1%     
=======================================
  Files        141     141             
  Lines       7464    7486     +22     
=======================================
+ Hits        5941    5966     +25     
+ Misses      1275    1271      -4     
- Partials     248     249      +1     
Impacted Files Coverage Δ
bridge/opentracing/bridge.go 47.8% <0.0%> (ø)
bridge/opentracing/wrapper.go 100.0% <ø> (ø)
trace/noop.go 46.1% <0.0%> (ø)
trace/trace.go 83.6% <ø> (ø)
metric/metric.go 52.5% <75.0%> (ø)
trace/config.go 75.5% <75.0%> (+14.9%) ⬆️
bridge/opentracing/internal/mock.go 72.7% <77.7%> (ø)
bridge/opencensus/bridge.go 84.9% <100.0%> (ø)
internal/global/meter.go 90.3% <100.0%> (ø)
internal/global/trace.go 83.7% <100.0%> (ø)
... and 9 more

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
@MadVikingGod MadVikingGod marked this pull request as ready for review May 20, 2021 14:36
@MadVikingGod MadVikingGod changed the title WIP Update API configs. Update API configs. May 20, 2021
@MrAlias MrAlias added this to the RC1 milestone May 20, 2021
metric/config.go Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Show resolved Hide resolved
trace/config.go Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
metric/config.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented May 26, 2021

I'm not sure about the lint failure, they look unrelated to the changes introduced here. Other than the syntax error this all looks good! 😄

@MadVikingGod
Copy link
Contributor Author

That is so strange. I got the error 123 once, but after re-running make I couldn't get it to reproduce. I also used the go test explorer extension, and all came back green.

@MrAlias
Copy link
Contributor

MrAlias commented May 27, 2021

That is so strange. I got the error 123 once, but after re-running make I couldn't get it to reproduce. I also used the go test explorer extension, and all came back green.

🤷

@MrAlias MrAlias merged commit c1f460e into open-telemetry:main May 27, 2021
@Aneurysm9 Aneurysm9 mentioned this pull request Jun 17, 2021
@MadVikingGod MadVikingGod deleted the fix-trace-config branch February 21, 2023 19:55
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.

Links provided to Span.End are silently dropped Unify Configuration and Initialization
3 participants