-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: Update OTEL dependencies to v0.112.0 #234
feat: Update OTEL dependencies to v0.112.0 #234
Conversation
8aeed3d
to
1120266
Compare
8753f0a
to
ad1b7cf
Compare
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.
Most of the changes look fine to me, just want to understand one change.
) | ||
|
||
func TestCreateDefaultConfig(t *testing.T) { | ||
assert.NoError(t, componenttest.CheckConfigStruct(NewFactory().CreateDefaultConfig())) | ||
} | ||
|
||
func TestCreateProcessor(t *testing.T) { | ||
mp, err := createMetricsProcessor(context.Background(), processor.CreateSettings{}, createDefaultConfig(), consumertest.NewNop()) | ||
cfg := createDefaultConfig().(*Config) |
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.
Could you explain why this was done? I'm having trouble understanding the reason.
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.
@braydonk Honestly I can't remember why I did this for the config...but good catch! Doesn't seem to be necessary. Might have came from a copy/paste while looking at existing factory tests within the latest release.
Pushing a change to just pass in createDefaultConfig()
in the 3 files where I did this.
ad1b7cf
to
b3a9524
Compare
ConfigProviderSettings: otelcol.ConfigProviderSettings{ | ||
ResolverSettings: confmap.ResolverSettings{ | ||
ProviderFactories: []confmap.ProviderFactory{ | ||
fileprovider.NewFactory(), |
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.
Is there a reason we're adding all these providers for our distro? Not sure why these are needed now
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.
Don't we only use the file provider?
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.
There was an upstream change that forces you do explicitly list providers now, I don't have a reference at hand though
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.
Ack, it stems from https://github.com/open-telemetry/opentelemetry-collector/pull/9513/files
We used to default to all the default providers so this change makes sense. We should separately reduce this list to just the file provider but that shouldn't block this PR
ConfigProviderSettings: otelcol.ConfigProviderSettings{ | ||
ResolverSettings: confmap.ResolverSettings{ | ||
ProviderFactories: []confmap.ProviderFactory{ | ||
fileprovider.NewFactory(), |
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.
Ack, it stems from https://github.com/open-telemetry/opentelemetry-collector/pull/9513/files
We used to default to all the default providers so this change makes sense. We should separately reduce this list to just the file provider but that shouldn't block this PR
This changes the OTEL dependencies to v0.112.0