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 WithTraceInfo that returns the logger with trace ID from the request #82

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

mariash
Copy link
Member

@mariash mariash commented Mar 15, 2023

Add a helper method to lager that returns a logger with trace information parsed from the request. We are planning to use this method across the components so it would be nice for logger to have it.

go.mod Outdated Show resolved Hide resolved
logger.go Outdated Show resolved Hide resolved
logger.go Outdated Show resolved Hide resolved
@ctlong ctlong moved this from Reviewer Assigned to Review in Progress in DEPRECATED App Platform - Logging and Metrics Mar 16, 2023
@ctlong
Copy link
Member

ctlong commented Mar 16, 2023

@mariash some thoughts:

❓Are you sure that this is the place to put this logic?
This logic seems very specific to an individual use-case for lager with zipkin in Cloud Foundry. Maybe it would be better to include it in a separate dependency that uses lager to emit the log line.

❓Are you intending to support more headers?
Word on the grapevine is that there may be more headers to support down the line, and if that's the case it would be nice if this method was setup to handle that right now.

❓Is everything that's going to be doing tracing happening in the context of an HTTP request?
It seems feasible that somewhere you may want to set trace-id or span-id properties that aren't contained in an http.Request. If so, it would be nice to make the input more generic and provide helper functions that do some of the trace ID / span ID extraction from http.Request.

@mariash
Copy link
Member Author

mariash commented Mar 17, 2023

@ctlong

❓Are you sure that this is the place to put this logic?

We thought it would be nice to have it here, since we will use this method in every request handler across different components. We can make a separate package that extends lager logger if there is a preference to not mix this into this package. I see the point that it is more specific to CF than other methods.

❓Are you intending to support more headers?

No we don't intend to support more headers.

❓Is everything that's going to be doing tracing happening in the context of an HTTP request?

So far I have not seen a use case for the request context. We usually have an http handler, that sets up logger session and then this logger is being passed in to other methods. If we happen to need it in the future we can modify the method to look up in the context and then in the header.

@mariash
Copy link
Member Author

mariash commented Mar 17, 2023

I am closing this PR, I think it makes sense to have this in a separate package since this is very CF specific method

@mariash mariash closed this Mar 17, 2023
DEPRECATED App Platform - Logging and Metrics automation moved this from Review in Progress to Done Mar 17, 2023
@winkingturtle-vmw winkingturtle-vmw merged commit 1e35ce1 into cloudfoundry:master Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants