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

Distributor: Don't use global logger in push handlers #6652

Merged
merged 4 commits into from
Nov 15, 2023
Merged

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Nov 14, 2023

What this PR does

Modify the Distributor's push handlers to receive the API's logger instead of using the global one (we're supposed to migrate off the global logger). A practical benefit from this is that the OTLP push handler receives the handler function's logger, which includes source IPs.

Which issue(s) this PR fixes or relates to

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 marked this pull request as ready for review November 14, 2023 13:57
@aknuds1 aknuds1 requested a review from a team as a code owner November 14, 2023 13:57
@aknuds1 aknuds1 added enhancement New feature or request component/distributor labels Nov 14, 2023
var decoderFunc func(buf []byte) (pmetricotlp.ExportRequest, error)

logger := log.WithContext(ctx, log.Logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can leave the log.WithContext, it adds traceid as part of logger, which spanlogger.NewWithLogger doesn't seems to do it.

Copy link
Contributor Author

@aknuds1 aknuds1 Nov 14, 2023

Choose a reason for hiding this comment

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

@ying-jeanne but handler calls this function (as parser) with a logger that's instantiated with request context. Do you agree that we no longer need the log.WithContext call in this function (since it's already been done by handler)?

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 requested a review from a team November 14, 2023 15:39
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

@aknuds1 aknuds1 merged commit 6a24a60 into main Nov 15, 2023
28 checks passed
@aknuds1 aknuds1 deleted the arve/otel-logger branch November 15, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants