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 correlation ids to api-server #346

Closed
sgerogia opened this issue Dec 16, 2021 · 2 comments
Closed

Add correlation ids to api-server #346

sgerogia opened this issue Dec 16, 2021 · 2 comments
Labels
self-contained Self-contained work, can be picked up independently of other tasks

Comments

@sgerogia
Copy link
Contributor

sgerogia commented Dec 16, 2021

The API server does not have a correlation / trace id to allow root cause analysis in the logs.
For example, the following 2 statements belong to the same request, yet they appear completely unrelated on first reading.
image.png

In addition, an optional external correlation id (i.e. defined by the client) will allow the debugging of complex interaction scenarios.
E.g. a number printed in the UI of the app, which users could quote to support to facilitate investigations.

For these reasons, this issue will create 2 correlation ids:

  • an internal one (int_correlation_id), and
  • an optional external one (correlation_id)

DoD

  • Introduce a correlation id middleware
  • Accept an optional request header X-Correlation-Id.
    If defined, use that value in the context as correlation_id. If not defined, ignore.
  • Also initialize an internal UUID and set in context. This will become the int_correlation_id.
  • In the middleware create a clone of the SuggaredLogger and standard Logger with the logging fields correlation-id and int_correlation_id (be careful of not creating a child that will not be GC'ed). Ensure that the logger emits logs with the new fields as standard.
  • Ensure that the code is using the cloned logger across the codebase.
  • The server returns the external correlation id as header X-Correlation-Id in the response (if defined).
  • Document the standard request/response header X-Correlation-Id in swagger_gen.go; ensure the header appears in the generated Swagger doc.
  • Unit tests for the external correlation id support
  • Unit tests for the internal correlation id support.
@sgerogia sgerogia added the self-contained Self-contained work, can be picked up independently of other tasks label Dec 17, 2021
@sgerogia sgerogia changed the title Add correlation id to api-server Add correlation ids to api-server Dec 20, 2021
@PrathyushaLakkireddy PrathyushaLakkireddy self-assigned this Jan 3, 2022
@gsora
Copy link
Contributor

gsora commented Jan 3, 2022

We are already attaching an ID to each error here, we might want to either get rid of this or refactor it to be the internal correlation ID.

@sgerogia
Copy link
Contributor Author

sgerogia commented Jan 31, 2022

Update

The scope of the task needs to be expanded, b/c of how our logging is setup.
image.png

  • There is a logging middleware function inside utils logging.LogRequest, which also needs to be updated.
  • Lazily look for a logger in context. If it exists, use that. If it does not and no explicit logger defined, panic. This can be an overloaded or separate function.
  • Move the correlation id middleware and GetLoggerFromContext to utils.
  • Use context logger in remaining locations of api-server (e.g. GetNumbersByAddress)

CC @SpideyPool192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
self-contained Self-contained work, can be picked up independently of other tasks
Projects
None yet
Development

No branches or pull requests

4 participants