-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[breaking][pkg/stanza] Pass TelemetrySettings to Build of Builder interface #32662
Merged
djaglowski
merged 1 commit into
open-telemetry:main
from
ChrsMark:pass_telemetry_settings_to_stanza_ops
Apr 24, 2024
Merged
[breaking][pkg/stanza] Pass TelemetrySettings to Build of Builder interface #32662
djaglowski
merged 1 commit into
open-telemetry:main
from
ChrsMark:pass_telemetry_settings_to_stanza_ops
Apr 24, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
cmd/configschema
configschema command
cmd/otelcontribcol
otelcontribcol command
cmd/oteltestbedcol
connector/datadog
exporter/datadog
Datadog components
exporter/elasticsearch
pkg/stanza
labels
Apr 24, 2024
github-actions
bot
requested review from
dineshg13,
djaglowski,
dmitryax,
JaredTan95,
liustanley,
mackjmr,
mx-psi,
songy23 and
ycombinator
April 24, 2024 06:13
ChrsMark
force-pushed
the
pass_telemetry_settings_to_stanza_ops
branch
13 times, most recently
from
April 24, 2024 08:48
753693b
to
efa37ae
Compare
djaglowski
reviewed
Apr 24, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ChrsMark. This looks good to me except for the unrelated logger import.
ChrsMark
force-pushed
the
pass_telemetry_settings_to_stanza_ops
branch
2 times, most recently
from
April 24, 2024 14:09
e024f86
to
a48bad4
Compare
…erface Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
ChrsMark
force-pushed
the
pass_telemetry_settings_to_stanza_ops
branch
from
April 24, 2024 14:14
a48bad4
to
ffdb8f6
Compare
djaglowski
approved these changes
Apr 24, 2024
djaglowski
pushed a commit
that referenced
this pull request
Apr 25, 2024
**Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> Telemetry settings in general are passed by value in the project. This commit aligns the stanza Builder interface with this convention. This will also ensures that Logger can be overwritten without changing the original object. Follow up of #32662. cc-ing @djaglowski who pointed that out. **Link to tracking Issue:** <Issue number if applicable> **Testing:** <Describe what testing was performed and which tests were added.> Same as #32662 **Documentation:** <Describe the documentation added.> Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
djaglowski
added a commit
that referenced
this pull request
May 16, 2024
#32662 updated many exported functions to accept `component.TelemetrySettings` instead of `zap.SugaredLogger`. This PR continues from there by passing `component.TelemetrySettings` deeper into the inner packages, and migrates from using `zap.SugaredLogger` to instead using `zap.Logger` (as provided by `component.TelemetrySettings`).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
cmd/configschema
configschema command
cmd/otelcontribcol
otelcontribcol command
cmd/oteltestbedcol
connector/datadog
exporter/datadog
Datadog components
exporter/elasticsearch
pkg/stanza
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR resumes the work from #31618.
The goal is to pass in the
component.TelemetrySettings
so as to use them later in various ways:SugaredLogger
toLogger
: [pkg/stanza] Switch from SugaredLogger to Logger #32177More about the breaking change decision at #31618 (comment).
Link to tracking Issue: #31256
Testing: Testing suite got updated.
Manual Testing
./bin/otelcontribcol_linux_amd64 --config ~/otelcol/config.yaml
echo 'some line' >> /var/log/busybox/refactroring_test.log
Documentation: TBA.