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

logger with context ? #1541

Closed
lichunqiang opened this issue Oct 12, 2021 · 8 comments · May be fixed by #1542
Closed

logger with context ? #1541

lichunqiang opened this issue Oct 12, 2021 · 8 comments · May be fixed by #1542

Comments

@lichunqiang
Copy link

Which version of Elastic are you using?

[ x ] elastic.v7 (for Elasticsearch 7.x)
[ ] elastic.v6 (for Elasticsearch 6.x)
[ ] elastic.v5 (for Elasticsearch 5.x)
[ ] elastic.v3 (for Elasticsearch 2.x)
[ ] elastic.v2 (for Elasticsearch 1.x)

Please describe the expected behavior

Can Logger support Context ? Because it's usefully for tracing,add requestId etc for a request.

Please describe the actual behavior

Any steps to reproduce the behavior?

@olivere
Copy link
Owner

olivere commented Oct 12, 2021

you can implement your own logger. Just implement the Logger interface as defined in https://github.com/olivere/elastic/blob/release-branch.v7/logger.go. Hope that helps.

@olivere olivere closed this as completed Oct 12, 2021
@lichunqiang
Copy link
Author

@olivere that does not meet the problem. (we have implement own logger with zap)

In our system, there is a "requestId" for per-request for logging, so we can combine multiple log for the same request.

if there is an error, errorlog will trace the error message, but we can not add custom fields to the log message (which get from Context).

@olivere
Copy link
Owner

olivere commented Oct 13, 2021

Ah. I see. Let me have another look at it.

@olivere olivere reopened this Oct 13, 2021
@olivere
Copy link
Owner

olivere commented Oct 14, 2021

Yep. Passing the context as part of the logging should be fine. Just need to make it backwards compatible.

As for tracing, there's already support for opentracing and opencensus, and logging is not the best way to do tracing.

olivere added a commit that referenced this issue Oct 14, 2021
This commit adds a `LoggerWithContext` interface that extends the
`Logger` interface by a method `PrintfWithContext` that, when
implemented, is called instead of the `Printf` method of the `Logger`
interface.

The purpose of `PrintfWithContext` is to receive the current context
under which the logging happens. Notice that this doesn't always
have to be request-scoped, i.e. an actual API call from a user. It may
also be from an internal state or process, e.g. Bulk processor or node
health.

Close #1541
olivere added a commit that referenced this issue Oct 14, 2021
This commit adds a `LoggerWithContext` interface that extends the
`Logger` interface by a method `PrintfWithContext` that, when
implemented, is called instead of the `Printf` method of the `Logger`
interface.

The purpose of `PrintfWithContext` is to receive the current context
under which the logging happens. Notice that this doesn't always
have to be request-scoped, i.e. an actual API call from a user. It may
also be from an internal state or process, e.g. Bulk processor or node
health.

Close #1541
@olivere
Copy link
Owner

olivere commented Oct 14, 2021

#1542 should give you what you want. I will leave it around until 7.0.30 will be released. There's a few things I don't like about that PR, but yeah. Maybe I'll find a cleaner solution.

If #1542 will be merged, you only have to implement LoggerWithContext interface to get a context.Context. Notice that that context may be anything: It's not only for requests being performed by any of the APIs: It's also for requests from background tasks (e.g. sniffing, healthchecks, or bulk processor). So don't rely on the fact that the context passed is actually from you.

@lichunqiang
Copy link
Author

@olivere thanks !

@EricDominguez1
Copy link

@olivere
hello, this change is not going to upload?

@olivere
Copy link
Owner

olivere commented Feb 11, 2023

Very probably not. This repository is effectively deprecated and dormant. Please switch to the official client.

@olivere olivere closed this as completed Feb 11, 2023
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 a pull request may close this issue.

3 participants