tests should also use ConfigPropertiesBackedConfigProvider #15805
Conversation
|
@trask please check |
| assertThat( | ||
| AutoConfigureUtil.getConfig(autoConfiguredSdk).getString("otel.exporter.otlp.protocol")) | ||
| .isEqualTo("grpc"); | ||
| assertThat(autoConfiguredSdk.getOpenTelemetrySdk().toString()) |
There was a problem hiding this comment.
Relying on toString isn't ideal. I don't really get the need to change this test like that. It is testing the property based configuration, what is wrong in using the ConfigurationProperties for this? Imo the old version of the test is more straightforward.
There was a problem hiding this comment.
- The goal for all production code is to use ConfigProvider instead of ConfigProperties.
- It's then confusing if some tests still use ConfigProperties.
- I'm not sure why
toString()is tested here - but lots of SDK tests do that - it's sort of a paradigm
There was a problem hiding this comment.
this is an interesting case. this test is here because we override the default otlp protocol in the Javaagent to be http/protobuf (instead of the SDK autoconfigure's default) of grpc.
so this test is very specific to ConfigProperties, and isn't needed for declarative configuration (where the default has been updated to http/protobuf and so we don't need to test that)
There was a problem hiding this comment.
I think we still want this test as long as we have ConfigProperties users, to ensure we don't break this change of the default value that we have implemented in the Java agent
zeitlinger
force-pushed
the
auto-conf-tests
branch
from
9ba0670 to
08a23c2
Compare
3 participants
to avoid testing an internal API (ConfigProperties)
split off from #15791