-
Notifications
You must be signed in to change notification settings - Fork 28
Conversation
newrelic/exporter_test.go
Outdated
span.SetAttributes(label.Int("depth", depth)) | ||
span.SetAttributes( | ||
attribute.Int("depth", depth), | ||
semconv.ServiceNameKey.String(serviceName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @akulnurislam ! This one has us scratching our heads a bit. What has changed to require us to add this serviceName in order for the test to pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RichVanderwal ya hahaha, I saw NewExporter
doesn't have any setAttributes.
If I look NewExportPipeline
there is set attribute serviceName for otel controller.
opentelemetry-exporter-go/newrelic/exporter.go
Lines 111 to 113 in e563425
// Minimally default resource with a service name. This is overwritten if | |
// another is passed in traceOpt or pushOpt. | |
r := resource.NewWithAttributes(semconv.ServiceNameKey.String(service)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RichVanderwal I've changed set attribute into NewTracerProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small non-blocking suggestion, everything else LGTM!
newrelic/exporter_test.go
Outdated
r := resource.NewWithAttributes(semconv.ServiceNameKey.String(serviceName)) | ||
|
||
tracerProvider := sdktrace.NewTracerProvider( | ||
sdktrace.WithBatcher(e, sdktrace.WithBatchTimeout(15), sdktrace.WithMaxExportBatchSize(10)), | ||
sdktrace.WithResource(r), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be nice to modify the example app like this as well (adding ServiceName resource here: https://github.com/newrelic/opentelemetry-exporter-go/blob/master/examples/simple/main.go#L57).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prmsrswt sure, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this version should go up, after the new release
github.com/newrelic/opentelemetry-exporter-go v0.17.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a new go.opentelemetry.io/otel
v0.19.0 has been released, should this PR be updated to the latest one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Merging this PR soon as a stepping stone to 0.19.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, @akulnurislam !
@akulnurislam @gunturaf @RichVanderwal @onprem @jodeev We have just released v0.18.0 of our Go OpenTelemetry Exporter which supports OpenTelemetry for Go v0.19.0. Let us know if you have any questions or comments. |
No description provided.