-
Notifications
You must be signed in to change notification settings - Fork 28
OpenTelemetry simple sample application #44
Conversation
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.
Noticed a couple of things.
examples/simple/main.go
Outdated
apiKey, ok := os.LookupEnv("NEW_RELIC_API_KEY") | ||
if !ok { | ||
fmt.Println("missing NEW_RELIC_API_KEY required for New Relic OpenTelemetry Exporter") | ||
} |
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.
Shouldn't we exit if there is no license key?
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 was thinking that if a customer was using this to retrofit an existing application, they wouldn't want to stop the world due to a lack of monitoring. I've added an os.Exit(1) in here now.
examples/simple/main.go
Outdated
} | ||
|
||
exporter, err := newrelic.NewExporter( | ||
"Sample OpenTelemetry 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.
The app name should be consistent with the folder name. We currently use simple
for the folder and sample
for the app name.
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.
Fixed, thanks.
examples/simple/main.go
Outdated
if err != nil { | ||
fmt.Printf("failed to instantiate New Relic OpenTelemetry exporter: %v\n", err) | ||
} |
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.
Shouldn't we exit if we are not able to instantiate the exporter?
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.
Same reasoning as above. Fixed. Perhaps we should be using log.Fatal() for these.
fooKey := label.Key("ex.com/foo") | ||
barKey := label.Key("ex.com/bar") | ||
lemonsKey := label.Key("ex.com/lemons") | ||
anotherKey := label.Key("ex.com/another") |
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.
Would it be helpful to change the prefix to simple
? It makes it obvious that the data came from the simple example.
Relatedly, what is the recommended practice in the OTel community for naming metrics? Do these metric names follow those practices?
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'm not familiar with metric naming practices in OpenTelemetry, but these metric names come straight from the Getting Started guide written by the OpenTelementry Go folks.
Changing it would add to the confusion.
It took @nr-swilloughby and I some time to figure out where the data was being sent and how to view it. It would be really helpful to include details in a short README on the steps needed to view the data. Something along the lines of:
Bonus points for screenshots for any of the above steps. |
I wish I had given you the context that this sample application is to supplement the Getting Started guide here. |
This sample application needs to be re-worked to accommodate the new API changes in Go OpenTelemetry v0.16.0. |
This is a very simple Go application, based off of @krnowak 's excellent Getting Started guide, which substitutes the New Relic OpenTelemetry Exporter for the
stdout
exporter in the original example.It's a supporting document for New Relic's OpenTelemetry documentation. Run it like this:
...where that key is obtained from the New Relic One UI according to these quick instructions.
It works for me, I'd love to hear others' experiences with it before it becomes part of our official documentation.