Sets trace ID in log entries correctly #683
Conversation
Adds a new logging enhancer that sets the right field for the log entries to be linkable from a trace, in the Google Cloud Console. Also updates Trace code sample to show this capability. Fixes #664
| You should see the trace information in detail. | ||
| Additionally, if you logged in with the Google Cloud SDK or have the `GOOGLE_CLOUD_PROJECT` environment variable set to your GCP project ID, you can also click the `View` link in front of the `Details` -> `Log` to view the log entries related to that trace ID. |
There was a problem hiding this comment.
This is really the only addition here. The rest is just putting sentences in the same line.
|
|
||
| @Override | ||
| public void enhanceLogEntry(LogEntry.Builder builder) { | ||
| String projectId = new DefaultGcpProjectIdProvider().getProjectId(); |
There was a problem hiding this comment.
If projectId is empty or null, there is really no suitable alternative, so it's safe if the final result is projects//traces/traceId or projects/null/traces/traceId.
There was a problem hiding this comment.
Is it really safe to call here new DefaultGcpProjectIdProvider()? What if I use fully different projectId strategy in my project?
May we somehow workaround with the system property or make that setCurrentTraceId aware about the projectId as well?
There was a problem hiding this comment.
Totally agree we should be doing this because the devsite mentions this format (https://cloud.google.com/trace/docs/viewing-details#log_entries) . But it makes me wonder why that other TraceLoggingEnhancer we were using even exists? the "trace_id" label that other one uses isn't mentioned in that devsite link at all... Should we set both ?
There was a problem hiding this comment.
If there's no harm in doing so, I think we should do both. In that case I think you could even extend TraceLoggingEnhancer from the client lib and just override that single method.
There was a problem hiding this comment.
I opened a new issue in google-cloud-java so they fix their current implementation. googleapis/google-cloud-java#3280
There was a problem hiding this comment.
We really only need the trace ID in one place, it's not being used for anything in labels.trace_id.
Probably better not to add any more complexity where it's not necessary.
| public void enhanceLogEntry(LogEntry.Builder builder) { | ||
| String projectId = new DefaultGcpProjectIdProvider().getProjectId(); | ||
| String traceId = getTraceId(); | ||
| if (traceId != null) { |
There was a problem hiding this comment.
Looks like if traceId == null, the projectId is redundant, therefore we even don't need to create a DefaultGcpProjectIdProvider instance in that case.
Just suggestion for minor optimization...
|
|
||
| @Override | ||
| public void enhanceLogEntry(LogEntry.Builder builder) { | ||
| String projectId = new DefaultGcpProjectIdProvider().getProjectId(); |
There was a problem hiding this comment.
Is it really safe to call here new DefaultGcpProjectIdProvider()? What if I use fully different projectId strategy in my project?
May we somehow workaround with the system property or make that setCurrentTraceId aware about the projectId as well?
There was a problem hiding this comment.
LGTM, but need to be rebased after #685 to be sure that Travis is OK with the changes.
Adds a new logging enhancer that sets the right field for the log
entries to be linkable from a trace, in the Google Cloud Console.
Also updates Trace code sample to show this capability.
Fixes #664