* Check that IDs round-trip between binary and hex string formats.
`TestIDsRoundTrip` is patterned after `TestNewIDs`.
* Check that bad values give the expected errors.
* Use larger values in `TestWithIDGenerator`. Previously, nearly all the
bits were zero so a mistake in encoding/decoding higher bits could be
missed.
Start with arbitrary values with more bits set.
(Span ID still has top half as zero due to taking an uint64)
* Modify `testIDGenerator` so this ^^ change runs on 32-bit platforms.
The idea for more tests arose while considering #6791.
Does not need a CHANGELOG entry - test only.
---------
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Fixes https://github.com/open-telemetry/opentelemetry-go/issues/7005
Adds `otel.sdk.processor.span.queue.size`,
`otel.sdk.processor.span.queue.capacity`, and
`otel.sdk.processor.span.processed.count` metrics to the trace batch
span processor.
These are defined in
cb11bb9bac/docs/otel/sdk-metrics.md,
and are experimental. Because of this, metrics are behind the
OTEL_GO_X_SELF_OBSERVABILITY feature gate.
Given the feature is experimental, it always uses the global
meterprovider when enabled.
---------
Co-authored-by: Robert Pająk <pellared@hotmail.com>
The unexported `version()` function in `sdk/trace` is not used anywhere.
Not mentioning that the value it is returning is wrong.
There is already a `Version` function in `sdk` (which is in the same Go
module).
### Description
OpenTelemetry uses `DefaultScheduleDelay` and `DefaultExportTimeout`
values as milliseconds but Go time package will understand them as
nanoseconds.
I understand that this is a stable library and that those value will
probably never change, so can we at least clarify their usage?
Right above the defaults declaration it says `// Defaults for
BatchSpanProcessorOptions.` which is confusing.
We used `trace.DefaultScheduleDelay` as a fallback value for our tracing
setup.
This confusion led to high CPU usage due to the frequent batch exports.
### Confusing behavior
```go
processor := trace.NewBatchSpanProcessor(exporter,
// set timeout to 5000 ns instead of the expected 5000 ms
trace.WithBatchTimeout(trace.DefaultScheduleDelay),
// set timeout to 30000 ns instead of the expected 30000 ms
trace.WithExportTimeout(trace.DefaultExportTimeout),
)
```
### Correct way to use those values
```go
processor := trace.NewBatchSpanProcessor(exporter,
trace.WithBatchTimeout(trace.DefaultScheduleDelay * time.Millisecond),
trace.WithExportTimeout(trace.DefaultExportTimeout * time.Millisecond),
)
```
---------
Co-authored-by: Damien Mathieu <42@dmathieu.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Commit 575e1bb270 deprecated the Library
type in favor of Scope, but did not add an empty line before the
deprecation comment. Go's formatting rules require an empty line;
omitting the empty line can cause some tools to not detect the
deprecation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
I would like to be able to use a private option in
https://github.com/open-telemetry/opentelemetry-go/pull/6393 in tests,
and decided to split this refactoring out into its own PR.
This moves the batch span processor benchmarks into benchmark_test.go,
and replaces one instance of the tracetest.NewInMemoryExporter with a
different test exporter implementation. It then moves most unit tests
from `trace_test` to the main `trace` package.
Fix#5996
### Correctness
From the [OTel
specification](88bffeac48/specification/common/README.md (attribute-limits)):
> - set an attribute value length limit such that for each attribute
value:
> - if it is a string, if it exceeds that limit (counting any character
in it as 1), SDKs MUST truncate that value, so that its length is at
most equal to the limit...
Our current implementation truncates on number of bytes not characters.
Unit tests are added/updated to validate this fix and prevent
regressions.
### Performance
```
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/trace
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
│ commit-b6264913(old).txt │ commit-54c61ac2(new).txt │
│ sec/op │ sec/op vs base │
Truncate/Unlimited-8 1.2300n ± 7% 0.8757n ± 3% -28.80% (p=0.000 n=10)
Truncate/Zero-8 2.341n ± 2% 1.550n ± 9% -33.77% (p=0.000 n=10)
Truncate/Short-8 31.6800n ± 3% 0.9960n ± 4% -96.86% (p=0.000 n=10)
Truncate/ASCII-8 8.821n ± 1% 3.567n ± 3% -59.57% (p=0.000 n=10)
Truncate/ValidUTF-8-8 11.960n ± 1% 7.163n ± 1% -40.10% (p=0.000 n=10)
Truncate/InvalidUTF-8-8 56.35n ± 0% 37.34n ± 18% -33.74% (p=0.000 n=10)
Truncate/MixedUTF-8-8 81.83n ± 1% 50.00n ± 1% -38.90% (p=0.000 n=10)
geomean 12.37n 4.865n -60.68%
│ commit-b6264913(old).txt │ commit-54c61ac2(new).txt │
│ B/op │ B/op vs base │
Truncate/Unlimited-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/Zero-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/Short-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/ASCII-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/ValidUTF-8-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/InvalidUTF-8-8 16.00 ± 0% 16.00 ± 0% ~ (p=1.000 n=10) ¹
Truncate/MixedUTF-8-8 32.00 ± 0% 32.00 ± 0% ~ (p=1.000 n=10) ¹
geomean ² +0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
│ commit-b6264913(old).txt │ commit-54c61ac2(new).txt │
│ allocs/op │ allocs/op vs base │
Truncate/Unlimited-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/Zero-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/Short-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/ASCII-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/ValidUTF-8-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/InvalidUTF-8-8 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
Truncate/MixedUTF-8-8 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
geomean ² +0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
```
#### Values shorter than limit
This is the default code path. Most attribute values will be shorter
than the default 128 limit that users will not modify.
The current code, `safeTruncate` requires a full iteration of the value
to determine it is valid and under the limit.
The replacement, `truncate`, first checks if the number of bytes in the
value are less than or equal to the limit (which guarantees the number
of characters are less than or equal to the limit) and returns
immediately. This will mean that invalid encoding less than the limit is
not changed, which meets the specification requirements.
#### Values longer than the limit
For values who's number of bytes exceeds the limit, they are iterated
only once with the replacement, `truncate`.
In comparison, the current code, `safeTruncate`, can iterate the string
up to three separate times when the string contains invalid characters.
```go
type interInst struct {
x int
}
type inter interface {
}
var sink []inter
func BenchmarkX(b *testing.B) {
sink = make([]inter, b.N)
for i := 0; i < b.N; i++ {
sink[i] = &interInst{}
}
clear(sink)
sink = sink[:0]
runtime.GC()
var ms runtime.MemStats
runtime.ReadMemStats(&ms)
b.Log(b.N, ms.Frees) // Frees is the cumulative count of heap objects freed.
}
```
```
clear:
ioz_test.go:35: 1 589
ioz_test.go:35: 100 711
ioz_test.go:35: 10000 10729
ioz_test.go:35: 1000000 1010750 <-- 1m+ freed
ioz_test.go:35: 16076874 17087643
ioz_test.go:35: 19514749 36602412
```
```
no clear:
ioz_test.go:35: 1 585
ioz_test.go:35: 100 606
ioz_test.go:35: 10000 725
ioz_test.go:35: 1000000 10745 <-- some "overhead" objects freed, not the slice.
ioz_test.go:35: 16391445 1010765
ioz_test.go:35: 21765238 17402230
```
This is documented at https://go.dev/wiki/SliceTricks:
> NOTE If the type of the element is a pointer or a struct with pointer
fields, which need to be garbage collected, the above implementations of
Cut and Delete have a potential memory leak problem: some elements with
values are still referenced by slice a’s underlying array, just not
“visible” in the slice. Because the “deleted” value is referenced in the
underlying array, the deleted value is still “reachable” during GC, even
though the value cannot be referenced by your code. If the underlying
array is long-lived, this represents a leak.
Followed by examples of how zeroing out the slice elements solves
the problem. This PR does the same.
Good day,
While working on
https://github.com/open-telemetry/opentelemetry-go/pull/5858 I found
some other possible improvements.
This PR:
- Adds an early return to `SetAttributes` when no attributes are
provided.
- Only increases `s.attributes` to guarantee that there is enough space
for elements to be added.
- Fixes and issue where `truncateAttr` was not used when a attribute was
being updated in `addOverCapAttrs`.
Thanks for reviewing and please let me know if any changes are needed.
---------
Co-authored-by: Damien Mathieu <42@dmathieu.com>
This benchmark was not only testing the batch processor, since it was
starting a span, and ending it.
So the entire process of recording span was being accounted.
This changes the benchmark to only account for the batch processor being
tested, not the entire stack.
Before:
```
pkg: go.opentelemetry.io/otel/sdk/trace
BenchmarkSpanProcessor-10 137320 8696 ns/op 11184 B/op 155 allocs/op
BenchmarkSpanProcessorVerboseLogging-10 135483 8881 ns/op 11184 B/op 155 allocs/op
PASS
ok go.opentelemetry.io/otel/sdk/trace 3.984s
```
After:
```
pkg: go.opentelemetry.io/otel/sdk/trace
BenchmarkSpanProcessorOnEnd/batch:_10,_spans:_10-10 6055572 173.2 ns/op 0 B/op 0 allocs/op
BenchmarkSpanProcessorOnEnd/batch:_10,_spans:_100-10 684236 1733 ns/op 0 B/op 0 allocs/op
BenchmarkSpanProcessorOnEnd/batch:_100,_spans:_10-10 6930391 173.8 ns/op 0 B/op 0 allocs/op
BenchmarkSpanProcessorOnEnd/batch:_100,_spans:_100-10 677128 1731 ns/op 0 B/op 0 allocs/op
BenchmarkSpanProcessorVerboseLogging-10 128823 9318 ns/op 11184 B/op 155 allocs/op
PASS
ok go.opentelemetry.io/otel/sdk/trace 6.763s
```
I haven't touched the verbose logging benchmark, as I suppose a
benchmark with verbose logging could actually benefit from the entire
stack.
I also feel one benchmark testing the entire stack up to there could be
nice.
I was looking at the trace benchmarks, and noticed this one, which says
it tests "span start", but ending the span is included within the data.
So this change removes ending the span from the computation, and adds a
new benchmark which only computes span end.
benchstat for span start:
```
pkg: go.opentelemetry.io/otel/sdk/trace
│ bench-main │ bench-branch │
│ sec/op │ sec/op vs base │
TraceStart-10 725.6n ± 3% 667.2n ± 2% -8.04% (p=0.000 n=10)
│ bench-main │ bench-branch │
│ B/op │ B/op vs base │
TraceStart-10 704.0 ± 0% 704.0 ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
│ bench-main │ bench-branch │
│ allocs/op │ allocs/op vs base │
TraceStart-10 14.00 ± 0% 14.00 ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
```
Benchmark for span end:
```
BenchmarkSpanEnd-10 16486819 147.7 ns/op 0 B/op 0 allocs/op
```
