-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
example/basic: make basic example work #279
Conversation
clsung
commented
Nov 2, 2019
- use stdout as exportor (otherwise no output when you run it)
- use stdout as exportor (otherwise no output when you run it)
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.
one minor nit. Otherwise LGTM.
example/basic/main.go
Outdated
func main() { | ||
initTracer() | ||
tracer := global.TraceProvider().GetTracer("ex.com/basic") | ||
meter := global.MeterProvider().GetMeter("ex.com/basic") // TODO: should share resources ^^^? |
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.
please add a note that Meter doesn't work yet.
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 rephrase TODO, 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.
FYI there is a Meter stdout exporter in #265, which also adds to this example.
It's fine to remove the note about flush.
For me, the TODO about resources is a spec-level "nice-to-have". It should be relatively easy to add resources to the meter and tracer in a coordinated way, but we haven't specified a way to do that. I believe the right approach would be to combine the two interfaces and support a common method for adding resources to a Tracer/Meter combo. |