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

Add WithClock TracerProviderOption #2052

Closed
wants to merge 41 commits into from
Closed
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
5d0f379
add clock/SetClock to trace provider (fix #2043)
Frefreak Jul 3, 2021
a8a08d4
fix lint (goimports)
Frefreak Jul 3, 2021
08351ed
fix missing license, modify changelog and fix test on windows
Frefreak Jul 4, 2021
6aaec19
adjustment
Frefreak Jul 6, 2021
33e4408
renaming, adjust changelog
Frefreak Jul 6, 2021
af73898
various change
Frefreak Jul 7, 2021
401d125
lint
Frefreak Jul 7, 2021
d41de7d
fix godoc, change CHANGELOG and unexport test struct
Frefreak Jul 7, 2021
39af369
fix changelog PR reference, fix typo and inaccurate wordings
Frefreak Jul 8, 2021
049cd2e
unexport `NowFunc` & `SinceFunc`
Frefreak Jul 8, 2021
dcc82b0
Update sdk/trace/provider.go
Frefreak Jul 8, 2021
e80a1fe
Update CHANGELOG.md
Frefreak Jul 8, 2021
8340380
add "stopwatch" to clock
Frefreak Jul 9, 2021
3f1a87d
improve doc
Frefreak Jul 9, 2021
f506ac3
Merge remote-tracking branch 'origin/main' into clock
Frefreak Jul 9, 2021
b9250b3
lint
Frefreak Jul 10, 2021
c7c088b
change interface signature
Frefreak Jul 10, 2021
d1a5ae0
small refactor
Frefreak Jul 10, 2021
51e400e
doc
Frefreak Jul 10, 2021
d5514dd
remove one unnecessary allocation
Frefreak Jul 21, 2021
04ef13c
Merge branch 'main' into clock
Frefreak Jul 21, 2021
8075f98
remove duplicated changelog items
Frefreak Jul 21, 2021
4f68db3
remove unsed defaultStopwatch
Frefreak Jul 21, 2021
beba0cb
rename Elapsed to Stop and modify documentation
Frefreak Jul 29, 2021
1cb2208
fix doc
Frefreak Jul 30, 2021
3aee73f
Merge branch 'main' into clock
Frefreak Jul 30, 2021
ccb0eda
Merge branch 'main' into clock
Frefreak Aug 4, 2021
bffe943
Update sdk/trace/time.go
Frefreak Aug 6, 2021
e2d7e8f
update test
Frefreak Aug 6, 2021
6173f5c
add nilStopwatch
Frefreak Aug 13, 2021
a81a903
refactor test
Frefreak Aug 13, 2021
670c671
refactor test again
Frefreak Aug 13, 2021
5206913
lowercase "should"
Frefreak Aug 13, 2021
9ef9a5a
fix doc
Frefreak Aug 26, 2021
e924492
pass user provided timestamp to stopwatch
Frefreak Sep 14, 2021
2f86f78
Merge branch 'main' into clock
Frefreak Sep 14, 2021
094b868
fix
Frefreak Sep 14, 2021
498dc4f
modify CHANGELOG and format
Frefreak Sep 14, 2021
1139987
Merge branch 'main' into clock
Frefreak Jan 19, 2022
bfc222e
move CHANGELOG content to unreleased
Frefreak Jan 19, 2022
e5c647d
fix vanity-import-check
Frefreak Jan 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Added

- The `"go.opentelemetry.io/otel/sdk/trace".TraceProvider` can now be configured with a newly added `Clock` interface.
This interface is called when determining the start and end times for a span.
Additionally, a `WithClock` is also added to the package enabling users to set custom implementations of the `Clock` interface.
The standard library `time` package is still used for this functionality by default if no option is set. (#2052)
- Added `ErrorHandlerFunc` to use a function as an `"go.opentelemetry.io/otel".ErrorHandler`. (#2149)

### Changed
Expand Down
12 changes: 0 additions & 12 deletions sdk/internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,10 @@ package internal // import "go.opentelemetry.io/otel/sdk/internal"

import (
"fmt"
"time"

"go.opentelemetry.io/otel"
)

// UserAgent is the user agent to be added to the outgoing
// requests from the exporters.
var UserAgent = fmt.Sprintf("opentelemetry-go/%s", otel.Version())

// MonotonicEndTime returns the end time at present
// but offset from start, monotonically.
//
// The monotonic clock is used in subtractions hence
// the duration since start added back to start gives
// end as a monotonic time.
// See https://golang.org/pkg/time/#hdr-Monotonic_Clocks
func MonotonicEndTime(start time.Time) time.Time {
return start.Add(time.Since(start))
}
28 changes: 26 additions & 2 deletions sdk/trace/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ import (
"sync/atomic"

"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/trace"

"go.opentelemetry.io/otel/sdk/instrumentation"
"go.opentelemetry.io/otel/sdk/resource"
"go.opentelemetry.io/otel/trace"
)

const (
Expand All @@ -51,6 +50,9 @@ type tracerProviderConfig struct {

// resource contains attributes representing an entity that produces telemetry.
resource *resource.Resource

// clock is used to provide start/end time for spans
clock Clock
}

type TracerProvider struct {
Expand All @@ -61,6 +63,7 @@ type TracerProvider struct {
idGenerator IDGenerator
spanLimits SpanLimits
resource *resource.Resource
clock Clock
}

var _ trace.TracerProvider = &TracerProvider{}
Expand Down Expand Up @@ -90,6 +93,7 @@ func NewTracerProvider(opts ...TracerProviderOption) *TracerProvider {
idGenerator: o.idGenerator,
spanLimits: o.spanLimits,
resource: o.resource,
clock: o.clock,
}

for _, sp := range o.processors {
Expand Down Expand Up @@ -330,6 +334,23 @@ func WithSpanLimits(sl SpanLimits) TracerProviderOption {
})
}

// WithClock returns a TracerProviderOption that will configure the
// TracerProvider's clock. The configured clock is used by Tracers
// to generate span start/end time. Clock.Stopwatch should start and
// return a Stopwatch instance. For Stopwatch implementation,
// Stopwatch.Started should return the time.Time when the stopwatch
// started. Stopwatch.Elapsed should return the time.Duration measuring
// the elapsed time from when the stopwatch started. Its value should be
// positive to ensure monotonic start/end time of the span.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It seems like this portion of the docs could be moved to their respective types/

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. I feels like the existing types' comment basically says the same thing so I remove most of the docs here.

//
// If this option is not used, the TracerProvider will provide the default
// clock which just calls the time package under the hood.
func WithClock(clk Clock) TracerProviderOption {
return traceProviderOptionFunc(func(cfg *tracerProviderConfig) {
cfg.clock = clk
})
}

// ensureValidTracerProviderConfig ensures that given TracerProviderConfig is valid.
func ensureValidTracerProviderConfig(cfg *tracerProviderConfig) {
if cfg.sampler == nil {
Expand All @@ -342,4 +363,7 @@ func ensureValidTracerProviderConfig(cfg *tracerProviderConfig) {
if cfg.resource == nil {
cfg.resource = resource.Default()
}
if cfg.clock == nil {
cfg.clock = defaultClock()
}
}
29 changes: 17 additions & 12 deletions sdk/trace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"go.opentelemetry.io/otel/trace"

"go.opentelemetry.io/otel/sdk/instrumentation"
"go.opentelemetry.io/otel/sdk/internal"
"go.opentelemetry.io/otel/sdk/resource"
)

Expand Down Expand Up @@ -112,9 +111,6 @@ type span struct {
// name is the name of this span.
name string

// startTime is the time at which this span was started.
startTime time.Time

// endTime is the time at which this span was ended. It contains the zero
// value of time.Time until the span is ended.
endTime time.Time
Expand Down Expand Up @@ -154,6 +150,9 @@ type span struct {

// spanLimits holds the limits to this span.
spanLimits SpanLimits

// stopwatch holds the Stopwatch returned by Clock.Start method
stopwatch Stopwatch
}

var _ trace.Span = &span{}
Expand All @@ -175,7 +174,7 @@ func (s *span) IsRecording() bool {
s.mu.Lock()
defer s.mu.Unlock()

return !s.startTime.IsZero() && s.endTime.IsZero()
return !s.startTime().IsZero() && s.endTime.IsZero()
}

// SetStatus sets the status of the Span in the form of a code and a
Expand Down Expand Up @@ -227,7 +226,7 @@ func (s *span) End(options ...trace.SpanEndOption) {

// Store the end time as soon as possible to avoid artificially increasing
// the span's duration in case some operation below takes a while.
et := internal.MonotonicEndTime(s.startTime)
et := s.stopwatch.Started().Add(s.stopwatch.Stop())
Frefreak marked this conversation as resolved.
Show resolved Hide resolved

// Do relative expensive check now that we have an end time and see if we
// need to do any more processing.
Expand Down Expand Up @@ -364,7 +363,7 @@ func (s *span) SpanKind() trace.SpanKind {
func (s *span) StartTime() time.Time {
s.mu.Lock()
defer s.mu.Unlock()
return s.startTime
return s.startTime()
}

// EndTime returns the time this span ended. For spans that have not yet
Expand Down Expand Up @@ -497,7 +496,7 @@ func (s *span) snapshot() ReadOnlySpan {
sd.resource = s.resource
sd.spanContext = s.spanContext
sd.spanKind = s.spanKind
sd.startTime = s.startTime
sd.startTime = s.startTime()
sd.status = s.status
sd.childSpanCount = s.childSpanCount

Expand Down Expand Up @@ -555,8 +554,14 @@ func (s *span) addChild() {

func (*span) private() {}

func (s *span) startTime() time.Time {
return s.stopwatch.Started()
}

func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.SpanConfig) *span {
span := &span{}
span := &span{
stopwatch: nilStopwatch{},
}

provider := tr.provider

Expand Down Expand Up @@ -614,10 +619,10 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.Sp

startTime := o.Timestamp()
if startTime.IsZero() {
startTime = time.Now()
span.stopwatch = tr.provider.clock.Stopwatch()
} else {
span.stopwatch = standardStopwatch(startTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the user supplies their own start time it shouldn't mean the user supplied clock is not used. This seems like a design flaw.

Should we updated the Clock interface's Stopwatch method to accept a start time?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess a given startTime might not always be useful or reasonable for some clock implementations (but I cannot really come up with any realistic examples). And I feels like this would complicate many implementations that could have been straight forward. Besides if the user provides a startTime and an endTime then the custom clock would not be used anyways.

Considering a "offset" clock, if user provide startTime, should the clock giving out the endTime based on the given startTime?

}
span.startTime = startTime

span.spanKind = trace.ValidateSpanKind(o.SpanKind())
span.name = name
span.parent = psc
Expand Down
65 changes: 65 additions & 0 deletions sdk/trace/time.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package trace

import (
"time"
)

// Clock is the entrypoint for providing time to span's start/end timestamp.
// By default the standard "time" package will be used. User can replace
// it with customized clock implementation (e.g. with additional clock
// synchronization logic) by using the `WithClock` option.
type Clock interface {
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
// Stopwatch returns a started Stopwatch measuring a time interval.
Stopwatch() Stopwatch
}

type Stopwatch interface {
Frefreak marked this conversation as resolved.
Show resolved Hide resolved
// Started returns the time the Stopwatch was started.
Started() time.Time
// Stop stops the stopwatch and returns the duration from when this Stopwatch was started.
// This will only be called once when generating span's end time and should return a positive
// time.Duration in order to ensure the monotonicity of span's start/end time.
Stop() time.Duration
}

type standardClock struct{}
type standardStopwatch time.Time
type nilStopwatch struct{}

func defaultClock() Clock {
return standardClock{}
}

func (standardClock) Stopwatch() Stopwatch {
return standardStopwatch(time.Now())
}

func (w standardStopwatch) Started() time.Time {
return time.Time(w)
}

func (w standardStopwatch) Stop() time.Duration {
return time.Since(time.Time(w))
}

func (w nilStopwatch) Started() time.Time {
return time.Time{}
}

func (w nilStopwatch) Stop() time.Duration {
return 0
}
43 changes: 43 additions & 0 deletions sdk/trace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1594,6 +1594,49 @@ func TestAddLinksWithMoreAttributesThanLimit(t *testing.T) {
}
}

type frozenClock struct {
now time.Time
}
type frozenStopwatch struct {
started time.Time
}

func (f frozenStopwatch) Started() time.Time {
return f.started
}
func (f frozenStopwatch) Stop() time.Duration {
return 0
}

// newFrozenClock returns a clock which stops at time t
func newFrozenClock(t time.Time) frozenClock {
return frozenClock{
now: t,
}
}

func (c frozenClock) Stopwatch() Stopwatch {
return frozenStopwatch{
started: c.now,
}
}

func TestCustomClock(t *testing.T) {
te := NewTestExporter()
now := time.Now()
tp := NewTracerProvider(WithSyncer(te), WithClock(newFrozenClock(now)))
tracer := tp.Tracer("custom-clock")

_, span := tracer.Start(context.Background(), "test-frozen-clock")
time.Sleep(time.Microsecond * 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look needed. Is there a reason to pause here?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC in windows if a span ends too fast it can result in the same start time/end time. If that happens then the Equal test would appears not that useful here.

span.End()
require.Equal(t, te.Len(), 1, "should only have one span")

got := te.Spans()[0]
assert.Equal(t, now, got.StartTime(), "StartTime should return the frozen time")
assert.Equal(t, now, got.EndTime(), "EndTime should return the frozen time")
}

type stateSampler struct {
prefix string
f func(trace.TraceState) trace.TraceState
Expand Down