closing and required parameters #6
Conversation
* As the GCP project id is required, it is less confusing to have it as a required parameter in order to benefit from compile time checks.
"github.com/golang/protobuf/ptypes/timestamp" | ||
gax "github.com/googleapis/gax-go" |
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 concern about gax
is : This project is currently experimental and not supported
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.
It's a dependency of the protobuf client we are using, not sure we can do much there. We are using the protobuf client directly here rather than the official StackDriver client.
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.
ok !
README.md
Outdated
if err != nil { | ||
log.Fatalf("error creating a recorder: %v", 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.
add defer recorder.Close()
here
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.
Added 👍
* Adds a close method for the recorder. Calling close flushes the buffered traces and closes the underlying client. * Inverts the dependency of the tracing client. Allows for easier testing and alternative implementations.
Hey,
I propose the following changes:
Add a
Close()
method for the Recorder. This will allow users to flush the buffers before their application terminates and thus avoid dropping traces.Make
Project ID
a required parameter. Recorder throws an error at the moment if WithProjectID("--") is not given which is a bit confusing. Making it a required parameter makes the need for this parameter explicit.Invert the dependency to Google's protobuf client. This allows for easier testing or alternative implementations. Building the client should not probably be the recorder's responsibility.