[AT-40][Add] Preliminary Logging CorrelationID #95
Conversation
edgegrid/log.go
Outdated
| log "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| var logBuffer *bufio.Writer | ||
| var LogFile *os.File | ||
| var EdgegridLog *log.Logger | ||
|
|
||
| // LogCorrelationID ID for header and footer of log file outputs | ||
| var LogCorrelationID *string |
There was a problem hiding this comment.
Shared variable will have trouble with concurrent calls.
There was a problem hiding this comment.
The same comments made in the provider AT-40 pull request apply here: I think the library should also be following a similar [level][api][guid] format. I would expect the common logging functions to be placed in the library itself and utilized in the provider.
| } | ||
| b, err := httputil.DumpRequestOut(req, body) | ||
| if err == nil { | ||
| LogMultiline(EdgegridLog.Traceln, string(b)) |
There was a problem hiding this comment.
Isn't this logging the same request two different ways? One as a single line out and another per line of the output? I like logging the request as a single action but am unable to grok why we still need it the old way as well. Can you explain?
There was a problem hiding this comment.
Ok I can remove these that was initial testing of wrapped logs I broke it out to separate function to allow incremental migration once correlation concept catches on
|
We don't need both. It won't hurt to always have a correlation ID. There is additional work to be done if the logic is simplified. All existing references will need to be tweaked including GTM and DNS. Since this is a library, we either need to require a correlation id, generate one if none provided or handle cases with and without. |
|
Looks like some of my comments did not post from last week. I didn't see any more glaring issues but I wasn't sure why some of the service calls pass empty log correlation ids. |
| } | ||
| b, err := httputil.DumpRequestOut(req, body) | ||
| if err == nil { | ||
| LogMultiline(EdgegridLog.Traceln, string(b)) |
There was a problem hiding this comment.
Same comment as above on PrintHtpRequest applies here. Also is the old endpoint for backwards compatibility and direct usage only? If so something like a deprecation warning should be added in the comments on the old function so users know to use the newer format.
There was a problem hiding this comment.
Ok I can remove these that was initial testing of wrapped logs I broke it out to separate function to allow incremental migration once correlation concept catches on
| @@ -15,7 +15,7 @@ var ( | |||
| // GetGroups retrieves all groups | |||
| func GetGroups() (*Groups, error) { | |||
| groups := NewGroups() | |||
| if err := groups.GetGroups(); err != nil { | |||
| if err := groups.GetGroups(""); err != nil { | |||
There was a problem hiding this comment.
All of these blank service calls, arent they problematic? Shouldn't they initialize a guid for sub-requests. I feel like i must be missing something here.
No description provided.