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 request id to logs. #4461

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add request id to logs. #4461

wants to merge 1 commit into from

Conversation

blkt
Copy link
Contributor

@blkt blkt commented Sep 12, 2024

Summary

This change adds a GRPC interceptor that fetches the header Grpc-Metadata-Request-Id and adds it to the logger. It also returns the request id to the client in the trailer, which is helpful for debugging.

CLI command minder history list was modified to show its usage, but those changes should be reverted.

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Manual tests.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@blkt blkt self-assigned this Sep 12, 2024
Comment on lines 131 to 134
fmt.Printf("%+v\n", rID)
fmt.Printf("%+v\n", header)
fmt.Printf("%+v\n", trailer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This obviously needs to be reverted, but I'm wondering what would be the right way to output the request id in the CLI.

@coveralls
Copy link

coveralls commented Sep 12, 2024

Coverage Status

coverage: 53.045% (-0.06%) from 53.105%
when pulling 4c55b9b on enh/request-id-interceptor
into 15fd53d on main.

@blkt blkt force-pushed the enh/request-id-interceptor branch 2 times, most recently from 71d4a69 to 9d1d981 Compare September 12, 2024 08:42
@blkt blkt changed the title Log request id to all messages. Add request id to logs. Sep 12, 2024
@blkt blkt marked this pull request as ready for review September 12, 2024 08:42
func maybeGetRequestID(ctx context.Context) string {
var rID string
if md, ok := metadata.FromIncomingContext(ctx); ok {
if rIDs, ok := md["request-id"]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

in HTTP what would this translate to? I wonder how it works with the HTTP gateway we use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GRPC moves back and forth headers having prefix Grpc-Metadata-, so it would become Grpc-Metadata-Request-Id.

@evankanderson
Copy link
Member

evankanderson commented Sep 17, 2024

@blkt blkt marked this pull request as draft September 19, 2024 11:27
@blkt
Copy link
Contributor Author

blkt commented Oct 2, 2024

@evankanderson while estimating work to improve logging I had a look at the documents you linked. My take is that the capabilities described in those specs far exceed our current need, which is to track a request execution from start to finish in logs.

My take is that we're better off polishing this work, possibly changing the header's key (not the value), and merge it.

@evankanderson
Copy link
Member

@evankanderson while estimating work to improve logging I had a look at the documents you linked. My take is that the capabilities described in those specs far exceed our current need, which is to track a request execution from start to finish in logs.

My take is that we're better off polishing this work, possibly changing the header's key (not the value), and merge it.

Sounds good; I'd thought this would be something like:

import "go.opentelemetry.io/otel/trace"

// ...
sc := trace.SpanContextFromContext(ctx)
ctx = zerolog.Ctx(ctx).With().Str("trace_id", sc.TraceID()).Logger().WithContext(ctx)

If there's a bunch more content, then I'd agree on doing our own thing.

This change adds a GRPC interceptor that fetches the header
`Grpc-Metadata-Request-Id` and adds it to the logger. It also returns
the request id to the client in the trailer, which is helpful for
debugging.

CLI command `minder history list` was modified to show its usage, but
those changes should be reverted.
@blkt blkt force-pushed the enh/request-id-interceptor branch from 8111143 to 4c55b9b Compare October 2, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants