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

component/otelcol: use zap adapter to accept logs from wrapped components #2323

Merged
merged 3 commits into from
Oct 4, 2022

Conversation

rfratto
Copy link
Member

@rfratto rfratto commented Oct 3, 2022

This introduces component/otelcol/internal/zapadapter, which creates a *zap.Logger instance from a github.com/go-kit/log.Logger instance. This is then used when creating OpenTelemetry Collector components, allowing us to continue to use github.com/go-kit/log consistently throughout Flow.

Related to #2213.

…ents

This introduces component/otelcol/internal/zapadapter, which creates a
*zap.Logger instance from a github.com/go-kit/log.Logger instance. This
is then used when creating OpenTelemetry Collector components, allowing
us to continue to use github.com/go-kit/log consistently throughout
Flow.

Related to grafana#2213.
Comment on lines +114 to +126
func (fe *fieldEncoder) AddArray(key string, marshaler zapcore.ArrayMarshaler) error {
// TODO(rfratto): allow this to write the value of the array instead of
// placeholder text.
fe.fields = append(fe.fields, fe.keyName(key), "<array>")
return nil
}

func (fe *fieldEncoder) AddObject(key string, marshaler zapcore.ObjectMarshaler) error {
// TODO(rfratto): allow this to write the value of the object instead of
// placeholder text.
fe.fields = append(fe.fields, fe.keyName(key), "<object>")
return nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't implement these here because:

  1. I think they're going to be a massive pain.
  2. I suspect logging arrays and objects is going to be fairly uncommon.

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

Some odd underlying things with Zap but overall LHTM.

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

LGTM

@rfratto rfratto merged commit f5c1d0d into grafana:main Oct 4, 2022
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Mar 17, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants