Add aws componenets benchmark #812

bhautikpip · 2021-06-03T10:23:42Z

Added benchmarks for aws idgenerator, propagator and for sampling and non-sampling scenarios

…-contrib into main

…y-go-contrib into main

…-contrib into main

…y-go-contrib into main

bhautikpip · 2021-06-07T06:20:38Z

@Aneurysm9 @MrAlias Can you review this PR?

propagators/aws/xray/idgenerator_test.go

propagators/aws/xray/sampled_unsampled_span_test.go

codecov · 2021-06-07T19:23:03Z

Codecov Report

Merging #812 (83fc5f6) into main (2ca6018) will increase coverage by 0.0%.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #812   +/-   ##
=====================================
  Coverage   78.7%   78.8%           
=====================================
  Files         62      62           
  Lines       2707    2707           
=====================================
+ Hits        2133    2135    +2     
+ Misses       441     440    -1     
+ Partials     133     132    -1

Impacted Files	Coverage Δ
...n/github.com/Shopify/sarama/otelsarama/producer.go	`94.8% <0.0%> (+2.0%)`	⬆️

propagators/aws/xray/sampled_unsampled_span_test.go

bhautikpip · 2021-06-08T07:31:07Z

@Aneurysm9 not sure but getting code cov failure in tests due to some missing token issue.

codecov/codecov-action#258

Aneurysm9 · 2021-06-10T16:42:20Z

@Aneurysm9 not sure but getting code cov failure in tests due to some missing token issue.

codecov/codecov-action#258

GitHub Actions was having a struggle the other day. I've kicked off the CI process again, we'll see if it's feeling better today!

MrAlias · 2021-06-15T18:33:14Z

propagators/aws/xray/sampled_unsampled_span_test.go

+		otel.SetTracerProvider(curTp)
+		otel.SetTextMapPropagator(curProp)


These are no-ops. The global set operation is only ever performed once.

https://github.com/open-telemetry/opentelemetry-go/blob/651409851900230cc2f8c62a5ec69a7ac9307847/internal/global/state.go#L74

https://github.com/open-telemetry/opentelemetry-go/blob/651409851900230cc2f8c62a5ec69a7ac9307847/internal/global/state.go#L50

It would make more sense to define tracer with TracerProvider in an init function and skip any dealing with the TextMapPropagtor (it doesn't look used in any of these benchmarks).

Are you suggesting to add trace.Start in init function ? As per @Aneurysm9's suggestion I have added restoring logic of propagators so had to add those in every benchmark.

I'm suggesting you create a TracerProvider in an init function that you then use to set the value of a package level tracer variable with.

The restoring logic you added does nothing, the second call is a no-op. You should not use the global tracer here.

MrAlias · 2021-06-15T18:40:35Z

propagators/aws/xray/sampled_unsampled_span_test.go

+	span.SetAttributes(attribute.Key("example attribute 2").String("value 2"))
+}
+
+func startAndEndUnSampledSpan() {


The created span in this function will come from a TracerProvider that has an AlwaysSample Sampler. This span will be sampled and this function is no different, as far as I can see, from startAndEndSampledSpan. Am I missing something?

Good callout. I will probably add tracer with NeverSample config.

MrAlias · 2021-06-18T00:01:46Z

I see my review has been re-requested, but the issues I raised on my first review have not been addressed AFAIK. Can you link me to the fixes if I missed them?

bhautikpip · 2021-06-18T09:41:39Z

I see my review has been re-requested, but the issues I raised on my first review have not been addressed AFAIK. Can you link me to the fixes if I missed them?

Yeah re-request review was done by mistake I still have to address your comments.

bhautikpip · 2021-06-18T16:20:53Z

I see my review has been re-requested, but the issues I raised on my first review have not been addressed AFAIK. Can you link me to the fixes if I missed them?

@MrAlias Submitted new changes to address your query please take a look.

MrAlias · 2021-06-18T16:55:48Z

propagators/aws/xray/sampled_unsampled_span_test.go

+	"go.opentelemetry.io/otel/trace"
+)
+
+var tracer = otel.Tracer("sample-app")


Suggested change

var tracer = otel.Tracer("sample-app")

var tracer trace.Tracer

MrAlias · 2021-06-18T16:57:51Z

propagators/aws/xray/sampled_unsampled_span_test.go

+	idg := NewIDGenerator()
+
+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithIDGenerator(idg),
+	)
+
+	otel.SetTracerProvider(tp)


Suggested change

idg := NewIDGenerator()

tp := sdktrace.NewTracerProvider(

sdktrace.WithSampler(sdktrace.AlwaysSample()),

sdktrace.WithIDGenerator(idg),

)

otel.SetTracerProvider(tp)

tracer = sdktrace.NewTracerProvider(

sdktrace.WithSampler(sdktrace.AlwaysSample()),

sdktrace.WithIDGenerator(NewIDGenerator()),

).Tracer("sample-app")

MrAlias · 2021-06-18T17:00:05Z

propagators/aws/xray/sampled_unsampled_span_test.go

+func startAndEndSampledSpan() {
+	var span trace.Span
+	_, span = tracer.Start(
+		context.Background(),
+		"Example Trace",
+	)
+
+	defer span.End()
+}


Suggested change

func startAndEndSampledSpan() {

var span trace.Span

_, span = tracer.Start(

context.Background(),

"Example Trace",

)

defer span.End()

}

MrAlias · 2021-06-18T17:01:24Z

propagators/aws/xray/sampled_unsampled_span_test.go

+	for i := 0; i < b.N; i++ {
+		startAndEndSampledSpan()
+	}


Suggested change

for i := 0; i < b.N; i++ {

startAndEndSampledSpan()

}

for i := 0; i < b.N; i++ {

_, span := tracer.Start(context.Background(), "Example Trace")

span.End()

}

MrAlias · 2021-06-18T17:01:47Z

propagators/aws/xray/sampled_unsampled_span_test.go

+func startAndEndNestedSampledSpan() {
+	var span trace.Span
+	ctx, span := tracer.Start(context.Background(), "Parent operation...")
+	defer span.End()
+
+	_, span = tracer.Start(ctx, "Sub operation...")
+	defer span.End()
+}


Suggested change

func startAndEndNestedSampledSpan() {

var span trace.Span

ctx, span := tracer.Start(context.Background(), "Parent operation...")

defer span.End()

_, span = tracer.Start(ctx, "Sub operation...")

defer span.End()

}

MrAlias · 2021-06-18T17:03:58Z

propagators/aws/xray/sampled_unsampled_span_test.go

+	for i := 0; i < b.N; i++ {
+		startAndEndNestedSampledSpan()
+	}


Suggested change

for i := 0; i < b.N; i++ {

startAndEndNestedSampledSpan()

}

ctx, parent := tracer.Start(context.Background(), "Parent operation...")

defer parent.End()

b.ResetTimer()

for i := 0; i < b.N; i++ {

_, span := tracer.Start(ctx, "Sub operation...")

span.End()

}

MrAlias · 2021-06-18T17:04:18Z

propagators/aws/xray/sampled_unsampled_span_test.go

+func getCurrentSampledSpan() trace.Span {
+	var span trace.Span
+	ctx, span := tracer.Start(
+		context.Background(),
+		"Example Trace",
+	)
+	defer span.End()
+
+	return trace.SpanFromContext(ctx)
+}


Suggested change

func getCurrentSampledSpan() trace.Span {

var span trace.Span

ctx, span := tracer.Start(

context.Background(),

"Example Trace",

)

defer span.End()

return trace.SpanFromContext(ctx)

}

MrAlias · 2021-06-18T17:06:27Z

propagators/aws/xray/sampled_unsampled_span_test.go

+func BenchmarkGetCurrentSampledSpan(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		getCurrentSampledSpan()
+	}
+}


This provides no additional bencmarking of this package. The creation and ending of a span is already benchmarked and this only adds a benchmark on retrieving a span from a context, something that is not a function of this package.

Suggested change

func BenchmarkGetCurrentSampledSpan(b *testing.B) {

for i := 0; i < b.N; i++ {

getCurrentSampledSpan()

}

}

MrAlias · 2021-06-18T17:07:32Z

propagators/aws/xray/sampled_unsampled_span_test.go

+func BenchmarkAddAttributesToSampledSpan(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		addAttributesToSampledSpan()
+	}
+}


Similar here, this tests the AddAttributes method of the default SDK and does not provide any relevant benchmark for this package.

Suggested change

func BenchmarkAddAttributesToSampledSpan(b *testing.B) {

for i := 0; i < b.N; i++ {

addAttributesToSampledSpan()

}

}

MrAlias · 2021-06-18T17:07:44Z

propagators/aws/xray/sampled_unsampled_span_test.go

+func addAttributesToSampledSpan() {
+	var span trace.Span
+	_, span = tracer.Start(
+		context.Background(),
+		"Example Trace",
+	)
+	defer span.End()
+
+	span.SetAttributes(attribute.Key("example attribute 1").String("value 1"))
+	span.SetAttributes(attribute.Key("example attribute 2").String("value 2"))
+}


Suggested change

func addAttributesToSampledSpan() {

var span trace.Span

_, span = tracer.Start(

context.Background(),

"Example Trace",

)

defer span.End()

span.SetAttributes(attribute.Key("example attribute 1").String("value 1"))

span.SetAttributes(attribute.Key("example attribute 2").String("value 2"))

}

MrAlias · 2021-06-18T17:09:40Z

propagators/aws/xray/sampled_unsampled_span_test.go

@@ -0,0 +1,104 @@
+// Copyright The OpenTelemetry Authors


This file should be renamed to something descriptive of the benchmarks included. I.e. idgenerator_benchmark_test.go

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

bhautikpip and others added 18 commits February 4, 2021 16:21

added failure scenario when getting container fails

dcc01de

fix test case failure

1dd54d9

add changelog

03f6fb4

Merge branch 'main' into main

5919be5

Merge branch 'main' into main

ebd45b4

fix ecs resource detector bug

e1ff7d0

Merge branch 'main' of https://github.com/bhautikpip/opentelemetry-go…

041d9b8

…-contrib into main

fix struct name as per golint suggestion

e12a4b1

fix merge conflict

e906d2a

minor changes

86c04d1

added NewResourceDetector func and interface assertions

047f5d0

fix golint failure

21db8fa

minor changes to address review comments

5204d27

Merge branch 'main' into main

c9c1bca

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

c956d4b

…y-go-contrib into main

Merge branch 'main' of https://github.com/bhautikpip/opentelemetry-go…

12c6c74

…-contrib into main

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

e042a6f

…y-go-contrib into main

added benchmarks for aws id generator and propagator

6b2e3c3

bhautikpip requested review from Aneurysm9, dashpole, evantorrie, jmacd, MadVikingGod, MrAlias, paivagustavo and XSAM as code owners June 3, 2021 10:23

This was referenced Jun 3, 2021

Move benchmark upstream aws-observability/aws-otel-go#24

Closed

remove benchmark aws-observability/aws-otel-go#71

Merged

bhautikpip added 2 commits June 3, 2021 03:44

ran gofmt

2622e4e

ran precommit

c50fed8

Aneurysm9 suggested changes Jun 7, 2021

View reviewed changes

propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved

propagators/aws/xray/sampled_unsampled_span_test.go Outdated Show resolved Hide resolved

bhautikpip added 2 commits June 7, 2021 11:48

remove idgenerator and added logic to restore global state

ac3b67a

added current state of propagator and trace provider

8fe79d4

bhautikpip requested a review from Aneurysm9 June 7, 2021 19:06

Aneurysm9 reviewed Jun 7, 2021

View reviewed changes

propagators/aws/xray/sampled_unsampled_span_test.go Outdated Show resolved Hide resolved

minor design change

0d0588f

bhautikpip requested a review from Aneurysm9 June 8, 2021 07:30

Aneurysm9 approved these changes Jun 10, 2021

View reviewed changes

MrAlias reviewed Jun 15, 2021

View reviewed changes

bhautikpip requested a review from MrAlias June 17, 2021 17:39

bhautikpip added 2 commits June 18, 2021 09:07

removed setting global propagators and minor changes

bd3d279

make precommit changes

79e7602

MrAlias reviewed Jun 18, 2021

View reviewed changes

removed unnecessary benchmark

c95f44a

bhautikpip requested a review from MrAlias June 18, 2021 17:40

MrAlias approved these changes Jun 18, 2021

View reviewed changes

Merge branch 'main' into add-aws-componenets-benchmark

83fc5f6

Aneurysm9 merged commit 91e5180 into open-telemetry:main Jun 18, 2021

bhautikpip mentioned this pull request Jul 22, 2021

REQUEST: New membership for @bhautikpip open-telemetry/community#790

Closed

6 tasks

plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022

Single-state Aggregator and test refactor (#812)

9925ebe

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

pellared added this to the untracked milestone Nov 8, 2024

MrAlias mentioned this pull request Jun 29, 2020

Upgrade to v0.7.0 of go.opentelemetry.io/otel #102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add aws componenets benchmark #812

Add aws componenets benchmark #812

bhautikpip commented Jun 3, 2021

bhautikpip commented Jun 7, 2021

codecov bot commented Jun 7, 2021 •

edited

Loading

bhautikpip commented Jun 8, 2021 •

edited

Loading

Aneurysm9 commented Jun 10, 2021

MrAlias Jun 15, 2021

bhautikpip Jun 17, 2021 •

edited

Loading

MrAlias Jun 18, 2021

MrAlias Jun 15, 2021

bhautikpip Jun 17, 2021

MrAlias commented Jun 18, 2021

bhautikpip commented Jun 18, 2021

bhautikpip commented Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

MrAlias Jun 18, 2021

		otel.SetTracerProvider(curTp)
		otel.SetTextMapPropagator(curProp)

	var tracer = otel.Tracer("sample-app")
	var tracer trace.Tracer

Add aws componenets benchmark #812

Add aws componenets benchmark #812

Conversation

bhautikpip commented Jun 3, 2021

bhautikpip commented Jun 7, 2021

codecov bot commented Jun 7, 2021 • edited Loading

Codecov Report

bhautikpip commented Jun 8, 2021 • edited Loading

Aneurysm9 commented Jun 10, 2021

Choose a reason for hiding this comment

bhautikpip Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAlias commented Jun 18, 2021

bhautikpip commented Jun 18, 2021

bhautikpip commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 7, 2021 •

edited

Loading

bhautikpip commented Jun 8, 2021 •

edited

Loading

bhautikpip Jun 17, 2021 •

edited

Loading