Fix/ub 1099 add request id to all logs

[olgashtivelman]

This change is 

[shay-berman]

Hi @olgashtivelman, Before the review please do:

1. Resolve the "conflicts" (you will see a button "resolve conflicts" that will help u to do it)
2. Please add to the PR a description that explain the PR and also link to the k8s PR that related to it as well.
3. Please do the same on the https://github.com/IBM/ubiquity-k8s/pull/191 as well. Thanks

[olgashtivelman]

the k8s review link. https://github.com/IBM/ubiquity-k8s/pull/191/

this is the change to add reqeuest_id and go routine id to the ubiquity + flex + provisioner logs so that we can follow a single request through out the product. (and also be able to identify which logs belong to which request in a multi-threaded server like ubiquity.)

[coveralls]

Coverage decreased (-0.3%) to 51.524% when pulling 475bd7056a726082d7c380e7bedd12677df6979e on fix/UB-1099_add_request_id_to_all_logs into a9fe98db89ece7dd4dd634e6e31bcdc3b77753fb on dev.

[shay-berman]

Review status: 0 of 8 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

remote/client.go, line 181 at r2 (raw file):

  detachRemoteURL := utils.FormatURL(s.storageApiURL, "volumes", detachRequest.Name, "detach")
  detachRequest.CredentialInfo = s.config.CredentialInfo
  response, err := utils.HttpExecute(s.httpClient, "PUT", detachRemoteURL, detachRequest, detachRequest.Context)

Olga, it doesn't make sense to add detachRequest,Context as another param for httpexecute, because the whole detachRequest is already passed, so it sending the same param twice.

this note relevant for all the changes here.

---

utils/http_utils.go, line 96 at r2 (raw file):

func HttpExecute(httpClient *http.Client, requestType string, requestURL string, rawPayload interface{}, request_context resources.RequestContext) (*http.Response, error) {
  logger := logs.GetLogger()
  payload, err := json.MarshalIndent(rawPayload, "", " ")

This is strange So you also marshal the request inside the post as well? so we actually passing twice the same data. Lets consider how to tune it.

---

utils/http_utils.go, line 108 at r2 (raw file):

  //setting the headers for the request
  request.Header.Set("request-id", request_context.Id)

ok here I would make it more generic, since request_context may additional keys in the future, then its better to add headers by on each key in the struct. So it will be ready framework and not specifically for this ID.

---

utils/logs/go_logging_logger.go, line 54 at r2 (raw file):

  b := make([]byte, 64)
  b = b[:

what is this red dots? not sure

---

utils/logs/go_logging_logger.go, line 64 at r2 (raw file):

var GoIdToRequestIdMap = new(sync.Map)

func (l *goLoggingLogger) getGoIdAndContextString() string {

rename function name "getContextStringFromGoId" so it will make more sense.

---

utils/logs/go_logging_logger.go, line 68 at r2 (raw file):

  context, exists := GoIdToRequestIdMap.Load(go_id)
  if !exists {
      context = resources.RequestContext{Id: "XXXXX"}

change it to "NA" (not applicable) instead of XXXX or "-". please mentioned it in the design wiki page.

---

utils/logs/go_logging_logger.go, line 72 at r2 (raw file):

      context = context.(resources.RequestContext)
  }
  return fmt.Sprintf("%d:%s", go_id, context.(resources.RequestContext).Id)

print first the requestConextID and then the go_id. its more important and its like father of the goid, so it should be before.

as mentioned in the design wiki page.

---

utils/logs/go_logging_logger.go, line 105 at r2 (raw file):

  switch level {
  case DEBUG:
      l.logger.Debug(fmt.Sprintf("[%s] %s", goid_context_string, traceEnter), args)

you can prepare the string (fmt.Sprintf("[%s] %s", goid_context_string, traceEnter)) before th switch and just use it on every function below, it will simplify the code here.

---

web_server/storage_api_handler.go, line 41 at r2 (raw file):

func getContextFromRequest(req *http.Request) resources.RequestContext {
  return resources.RequestContext{Id: req.Header.Get("request-id")}

I would do here also something generic that will go on all the expected keys and not only request-id.

BTW consider what happened if its not there, you should consider to return empty string.

---

Comments from Reviewable

[olgashtivelman]

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

remote/client.go, line 181 at r2 (raw file):

Previously, shay-berman wrote…

Olga, it doesn't make sense to add detachRequest,Context as another param for httpexecute, because the whole detachRequest is already passed, so it sending the same param twice. this note relevant for all the changes here.

yes you are right. meant to change it.

---

utils/http_utils.go, line 96 at r2 (raw file):

Previously, shay-berman wrote…

This is strange So you also marshal the request inside the post as well? so we actually passing twice the same data. Lets consider how to tune it.

this is not a new change from this review. not sure why we are parsing twice? where?

---

utils/http_utils.go, line 108 at r2 (raw file):

Previously, shay-berman wrote…

ok here I would make it more generic, since request_context may additional keys in the future, then its better to add headers by on each key in the struct. So it will be ready framework and not specifically for this ID.

I think that per your comment above i will remove the context from the header and only use it in the request as it is sent there anyways.

---

utils/logs/go_logging_logger.go, line 54 at r2 (raw file):

Previously, shay-berman wrote…

what is this red dots? not sure

what red dots? (I don't see any dots in the reiview..)

---

utils/logs/go_logging_logger.go, line 64 at r2 (raw file):

Previously, shay-berman wrote…

rename function name "getContextStringFromGoId" so it will make more sense.

Done.

---

utils/logs/go_logging_logger.go, line 68 at r2 (raw file):

Previously, shay-berman wrote…

change it to "NA" (not applicable) instead of XXXX or "-". please mentioned it in the design wiki page.

Done.

---

utils/logs/go_logging_logger.go, line 72 at r2 (raw file):

Previously, shay-berman wrote…

print first the requestConextID and then the go_id. its more important and its like father of the goid, so it should be before. as mentioned in the design wiki page.

in the design wiki page the go-id is first... But I can change it in both.

---

utils/logs/go_logging_logger.go, line 105 at r2 (raw file):

Previously, shay-berman wrote…

you can prepare the string (fmt.Sprintf("[%s] %s", goid_context_string, traceEnter)) before th switch and just use it on every function below, it will simplify the code here.

Done.

---

web_server/storage_api_handler.go, line 41 at r2 (raw file):

Previously, shay-berman wrote…

I would do here also something generic that will go on all the expected keys and not only request-id. BTW consider what happened if its not there, you should consider to return empty string.

as mentioned above I am adding a new commit with the change of not sending the request id also in the header as you mentioned we send it twice. so this is no longer relevant.

---

Comments from Reviewable

[olgashtivelman]

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

remote/client.go, line 181 at r2 (raw file):

Previously, olgashtivelman wrote…

yes you are right. meant to change it.

Done.

---

utils/http_utils.go, line 108 at r2 (raw file):

Previously, olgashtivelman wrote…

I think that per your comment above i will remove the context from the header and only use it in the request as it is sent there anyways.

Done.

---

web_server/storage_api_handler.go, line 41 at r2 (raw file):

Previously, olgashtivelman wrote…

as mentioned above I am adding a new commit with the change of not sending the request id also in the header as you mentioned we send it twice. so this is no longer relevant.

Done.

---

Comments from Reviewable

[olgashtivelman]

shay-berman please review the code changes and the comments.

---

Review status: 0 of 9 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[olgashtivelman]

@shay-berman please review the code changes and the comments.

---

Review status: 0 of 9 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[shay-berman]

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

remote/client.go, line 181 at r2 (raw file):

Previously, olgashtivelman wrote…

Done.

but i don't see the fix? i still see it twice (commit and put to the branch)

---

utils/http_utils.go, line 98 at r4 (raw file):

  payload, err := json.MarshalIndent(rawPayload, "", " ")

  logger.Info("Sending HTTPExecute request")

why its info? i think it should be debug or maybe its not needed at all here. if u want to keep it in debug then please move it few lines below to be close to the real newrequest function.

---

utils/logs/go_logging_logger.go, line 72 at r2 (raw file):

Previously, olgashtivelman wrote…

in the design wiki page the go-id is first... But I can change it in both.

great its better to see the reqID first

---

Comments from Reviewable

[olgashtivelman]

Review status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @shay-berman and @olgashtivelman)

---

utils/http_utils.go, line 98 at r4 (raw file):

Previously, shay-berman wrote…

why its info? i think it should be debug or maybe its not needed at all here. if u want to keep it in debug then please move it few lines below to be close to the real newrequest function.

I guess its here by mistake when I was trying to test something. i removed it.

---

remote/client.go, line 181 at r2 (raw file):

Previously, shay-berman wrote…

but i don't see the fix? i still see it twice (commit and put to the branch)

now you can see it

---

Comments from Reviewable

[shay-berman]

---

Reviewed 1 of 8 files at r1, 3 of 6 files at r2, 1 of 3 files at r4, 4 of 4 files at r5. Review status: all files reviewed, 1 unresolved discussion (waiting on @shay-berman)

---

utils/http_utils.go, line 98 at r4 (raw file):

Previously, olgashtivelman wrote…

I guess its here by mistake when I was trying to test something. i removed it.

great, this is why we do code review :-) and you review mine

---

Comments from Reviewable