Skip to content
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

Add trace ID to log metadata when available #2574

Merged
merged 2 commits into from
Sep 8, 2017

Conversation

draffensperger
Copy link
Contributor

@draffensperger draffensperger commented Aug 30, 2017

This makes the logging-bunyan and logging-winston libraries add the LogEntry.trace field based on the current trace context of the @google-cloud/trace-agent library when available.

Users can override that default LogEntry.trace assignment by specifying a logging.googleapis.com/trace field in the Winston entry metadata or Bunyan structured log entry. That naming makes logging backward-compatible for anyone who might happen to already use a trace field in their logs and it the follows the similar behavior for the Stackdriver logging agent, see https://github.com/GoogleCloudPlatform/fluent-plugin-google-cloud/blob/91835bb12b728ba88f63c38008e539a047d216a6/lib/fluent/plugin/out_google_cloud.rb#L105.

This will make use of the getWriterProjectId function that googleapis/cloud-trace-nodejs#548 adds to cloud-trace-nodejs.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 30, 2017
@draffensperger draffensperger force-pushed the trace-ids-in-logs branch 3 times, most recently from db4035f to 1b374f6 Compare September 1, 2017 13:46
@draffensperger draffensperger force-pushed the trace-ids-in-logs branch 2 times, most recently from cc052c7 to 67ce8c3 Compare September 1, 2017 14:46
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 99.976% when pulling 67ce8c3 on draffensperger:trace-ids-in-logs into 4292811 on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0de66c1 on draffensperger:trace-ids-in-logs into 4292811 on GoogleCloudPlatform:master.

@draffensperger
Copy link
Contributor Author

@matthewloring and @ofrobots could you take a look at this when you get a chance?

The getCurrentTraceFromAgent function is duplicated in both logging-bunyan and logging-winston, but it's pretty short and the alternative would be to move it to the core logging library, but that would require multiple PRs and updating dependencies (which I'm open to do if you prefer).

Copy link
Contributor

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of the trace agent looks good. Duplicating the qualified trace ID logic in winston and bunyan separately does seem clunky but if the update -> release -> bump dependency flow is too cumbersome then this is fine.

@@ -133,8 +140,6 @@ LoggingBunyan.prototype.formatEntry_ = function(record) {
);
}

record = extend({}, record);

This comment was marked as spam.

This comment was marked as spam.

* request, and to store as an intermediate value on the log entry before it
* gets written to the Stackdriver logging API.
*/
var LOGGING_TRACE_KEY = 'logging.googleapis.com/trace';

This comment was marked as spam.

This comment was marked as spam.

* @google-cloud/trace-agent library in the LogEntry.trace field format of:
* "projects/[PROJECT-ID]/traces/[TRACE-ID]".
*/
function getCurrentTraceFromAgent() {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@ofrobots
Copy link
Contributor

ofrobots commented Sep 8, 2017 via email

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for being patient on this!

@ofrobots
Copy link
Contributor

ofrobots commented Sep 8, 2017

@stephenplusplus Is this good to land from your POV?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. api: logging-bunyan api: logging-winston cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants