Update OTel libraries to v0.100.0 #208
Conversation
.gitignore
Outdated
|
|
||
| # GPU receiver generated component tests | ||
|
|
||
| # These generated test files do not work with the default off build tag setup |
There was a problem hiding this comment.
Is this considered breakage? Something OTel plans to fix?
There was a problem hiding this comment.
Not a breakage. We do a very atypical pattern here. We could probably rearchitect it but to just get one unit test to pass I'm not convinced it's worth it. If we do ever decide to build otelopscol with the OpenTelemetry Collector Builder instead in the future we may be able to come up with a better way, but decided this was easier for now.
There was a problem hiding this comment.
Ok, the current phrasing confused me. Maybe something like:
# These generated test files were not designed to work with the "default off"
# build tag approach we use for the GPU receivers. As such…
?
There was a problem hiding this comment.
Rephrased in 0e95bd9
internal/tools/go.mod
Outdated
| go 1.20 | ||
| go 1.21 | ||
|
|
||
| toolchain go1.21.9 |
There was a problem hiding this comment.
Hmm, presubmit.yml uses 1.21.10. Should it be consistent?
There was a problem hiding this comment.
toolchain directive represents the minimum required, so this doesn't provide any negatives. I was using a 1.21.9 toolchain when I made this update so that's why it kept getting automatically added here. Realistically it could probably be changed to 1.21.0 because we don't care about the particular patch version
There was a problem hiding this comment.
I'm just wondering if we should be testing with the minimum version we require…
There was a problem hiding this comment.
Updated this toolchain to be go1.21.10 since we might as well just stay consistent with the latest 1.21 available. Done in f224b1b
receiver/mongodbreceiver/config.go
Outdated
| @@ -59,7 +60,7 @@ func (c *Config) Validate() error { | |||
| err = multierr.Append(err, errors.New("password provided without user")) | |||
| } | |||
|
|
|||
| if _, tlsErr := c.LoadTLSConfig(); tlsErr != nil { | |||
| if _, tlsErr := c.LoadTLSConfig(context.TODO()); tlsErr != nil { | |||
There was a problem hiding this comment.
Just curious, why not context.Background()?
There was a problem hiding this comment.
I was following the recommendations of this go-staticcheck warning. Realistically it does not make a difference, as the context in this function is an _ arg.
There was a problem hiding this comment.
To me, context.TODO() implies "revisit later and replace with a proper context". Given your second sentence, context.Background() sounds like the proper context (same as context.TODO(), but without the implications), but up to you.
There was a problem hiding this comment.
Changed in 37653b1
| // limitations under the License. | ||
|
|
||
| //go:build gpu | ||
| // +build gpu |
There was a problem hiding this comment.
Do we want to add a comment here that this file is basically the generated test with the build tag added? Ditto for receiver/nvmlreceiver/component_test.go?
Updating OpenTelemetry to v0.100.0. This involved the following changes:
component.Typeinstead ofstringfor creating new components in factoriesconfignettypes [confignet] Should we change the name ofNetAddrandNetTCPto end inConfig? open-telemetry/opentelemetry-collector#9509go.opentelemetry.iolocation formdatagenpackagegooglemanagedprometheusexporterandprometheusreceiverand pull the upstream versions insteadmetadata.yamlfiles to usetypeinstead ofnameand includestabilityinternal/metadatapackages and adjust to new APIs (this has been skipped a couple releases in the GPU receivers due to a badmaketarget so the changes were more substantial there)loggingexportertodebugexporterasloggingexporteris deprecated and will be deleted soongenerated_component_test.gofiles in the GPU receivers, instead make a copy of them and use the proper build tags