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

Unify API Span Start/End Options #1108

Merged
merged 18 commits into from
Sep 3, 2020
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 28, 2020

  • Replace StartOption and EndOption in the trace API with SpanOption.
  • Add a unified SpanConfig to match and a SpanConfigure function to parse a SpanConfig from SpanOptions.
  • Replace the func LinkedTo(sc SpanContext, attrs ...label.KeyValue) StartOption with func WithLinks(links ...Link) SpanOption. This is more consistent with our other option patterns and a cleaner function signature.

Resolves #988
Part of #536

Replace both with `SpanOption`. Add a unified `SpanConfig` to match and
a `SpanConfigure` function to parse a `SpanConfig` from `SpanOption`s.

Update all the related options to use new `SpanOption`s.
The SDK uses an internal clock for the current time that cannot be use
if it does not know the time has not been set.
This preserves existing behavior.
Include SDK implementation specific information in the Tracer Start
method and Span End method.
@MrAlias MrAlias added pkg:API Related to an API package area:trace Part of OpenTelemetry tracing release:required-for-ga labels Aug 28, 2020
@MrAlias MrAlias self-assigned this Aug 28, 2020
@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #1108 into master will decrease coverage by 0.1%.
The diff coverage is 96.1%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1108     +/-   ##
========================================
- Coverage    77.2%   77.0%   -0.2%     
========================================
  Files         133     132      -1     
  Lines        7113    6984    -129     
========================================
- Hits         5492    5383    -109     
+ Misses       1371    1359     -12     
+ Partials      250     242      -8     
Impacted Files Coverage Δ
internal/trace/mock_span.go 10.0% <0.0%> (ø)
bridge/opentracing/bridge.go 40.4% <81.8%> (+0.1%) ⬆️
api/global/internal/trace.go 91.3% <100.0%> (ø)
api/trace/api.go 56.6% <100.0%> (-7.1%) ⬇️
api/trace/noop_span.go 20.0% <100.0%> (ø)
api/trace/noop_trace.go 100.0% <100.0%> (ø)
api/trace/tracetest/span.go 96.1% <100.0%> (-0.1%) ⬇️
api/trace/tracetest/tracer.go 100.0% <100.0%> (ø)
bridge/opentracing/internal/mock.go 67.8% <100.0%> (+0.4%) ⬆️
bridge/opentracing/wrapper.go 100.0% <100.0%> (ø)
... and 15 more

@MrAlias MrAlias marked this pull request as ready for review August 28, 2020 17:40
Copy link
Contributor

@evantorrie evantorrie left a comment

Choose a reason for hiding this comment

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

Code looks fine. Just some typo/wording nits.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
api/trace/api.go Outdated Show resolved Hide resolved
api/trace/api.go Outdated Show resolved Hide resolved
api/trace/api.go Outdated Show resolved Hide resolved
api/trace/api.go Outdated Show resolved Hide resolved
@MrAlias MrAlias requested a review from XSAM as a code owner August 31, 2020 16:07
Try for a less tautological comment.
Copy link
Contributor

@evantorrie evantorrie left a comment

Choose a reason for hiding this comment

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

LGTM.

api/trace/api.go Show resolved Hide resolved
@MrAlias MrAlias merged commit d143b8f into open-telemetry:master Sep 3, 2020
@MrAlias MrAlias deleted the span-config branch September 3, 2020 14:34
evantorrie added a commit to evantorrie/opentelemetry-go that referenced this pull request Sep 10, 2020
* Unify API Span Start/End Options

Replace both with `SpanOption`. Add a unified `SpanConfig` to match and
a `SpanConfigure` function to parse a `SpanConfig` from `SpanOption`s.

Update all the related options to use new `SpanOption`s.

* No non-zero SpanConfig defaults

The SDK uses an internal clock for the current time that cannot be use
if it does not know the time has not been set.

* Append attributes for WithAttributes

This preserves existing behavior.

* Add unit test for SpanConfigure

* Propagate changes

* Update append option documentation

* Update testing comments

* Move comments on guarantees to appropriate function

* Add documentation for SDK methods

Include SDK implementation specific information in the Tracer Start
method and Span End method.

* Add changes to Changelog

* Apply suggestions from code review

Co-authored-by: ET <[email protected]>

* Update the SpanKind comment in the  SpanConfig

Try for a less tautological comment.

Co-authored-by: ET <[email protected]>
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing pkg:API Related to an API package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify API Span Start/End Options
4 participants