[receiver/otlpreceiver] Use configoptional type#13119
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (94.11%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #13119 +/- ##
==========================================
- Coverage 91.60% 91.59% -0.01%
==========================================
Files 506 506
Lines 28544 28533 -11
==========================================
- Hits 26147 26136 -11
Misses 1882 1882
Partials 515 515 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mx-psi
requested review from
atoulme,
evan-bradley,
jade-guiton-dd and
yurishkuro
|
I think this one should not have any controversy ( |
| func GetOrInsertDefault[T any](t *testing.T, opt *configoptional.Optional[T]) *T { | ||
| if opt.HasValue() { | ||
| return opt.Get() | ||
| } | ||
|
|
||
| empty := confmap.NewFromStringMap(map[string]any{}) | ||
| require.NoError(t, empty.Unmarshal(opt)) | ||
| val := opt.Get() | ||
| require.NotNil(t, "Expected a default value to be set for %T", val) | ||
| return val | ||
| } |
There was a problem hiding this comment.
I think this is useful enough to be part of the Optional type API, something like what the mo package provides:
// MapNone executes the mapper function if value is absent or returns Option.
func (o Optional[T]) MapNone(mapper func() (T, bool)) Optional[T] {
if o.hasValue {
return Some(o.value)
}
return ToOption(mapper())
}(no need to change this PR)
There was a problem hiding this comment.
Yeah originally in one of my PoCs this was a method in Optional itself, but I got feedback that if it was only used in tests we could just not have it as part of configoptional. I think it may make sense to add it to the main API/a test module, but since we don't have to decide that now, I would rather keep it here
| if !r.cfg.GRPC.HasValue() { | ||
| return nil | ||
| } | ||
|
|
||
| grpcCfg := r.cfg.GRPC.Get() |
There was a problem hiding this comment.
We could simplify these cases by extending the Get() to return (T, bool) instead.
There was a problem hiding this comment.
Considering Get() returns a *T, I think you could just check whether it's nil.
There was a problem hiding this comment.
That is a good point. Since that does not change the API substantially I would prefer also doing this in a separate PR. I'll wait to see what others say and file an issue to address this before we mark configoptional 1.0
There was a problem hiding this comment.
What about creating another function for pointer return ToPointer and Get returning the value? The return type could be a private empty one return empty[T](), false
There was a problem hiding this comment.
Considering
Get()returns a*T, I think you could just check whether it's nil.
I mean, yeah, but that is not the usual Go pattern
There was a problem hiding this comment.
I was thinking about this during the PR for adding the configoptional package. I like that HasValue gates the rest of the function and avoids nesting, but I see the value in the other approaches as well.
My thought was that we see how this is used in a few places and see which pattern ends up being the cleanest for the typical case. From what I can tell, outside the reduced API of removing HasValue and checking whether *T is nil or ok is true for (T, bool), none of the options are substantially cleaner than the others.
evan-bradley
left a comment
evan-bradley
left a comment
There was a problem hiding this comment.
Sorry for the slow review. I like the way this looks.
|
I am going to merge this, although I want to make some changes to |
e8ca607
otlpreceiver.Config.GRPC and .HTTP are now configoptional.Optional[T] initialized in the Default flavor by CreateDefaultConfig(). Default.Get() returns nil, so the previous code using .Get() silently skipped endpoint and TLS assignment. Replaced .Get() with .GetOrInsertDefault() which promotes Default → Some and returns a mutable pointer. Extracted TLS config into a shared variable that defaults to None (zero value) when no cert/key files are provided. Upstream PRs: - open-telemetry/opentelemetry-collector#13044 (configoptional module) - open-telemetry/opentelemetry-collector#13119 (otlpreceiver migration) - open-telemetry/opentelemetry-collector#13252 (configgrpc TLS migration)
otlpreceiver.Config.GRPC and .HTTP are now configoptional.Optional[T] initialized in the Default flavor by CreateDefaultConfig(). Default.Get() returns nil, so the previous code using .Get() silently skipped endpoint and TLS assignment. Replaced .Get() with .GetOrInsertDefault() which promotes Default → Some and returns a mutable pointer. Extracted TLS config into a shared variable that defaults to None (zero value) when no cert/key files are provided. Upstream PRs: - open-telemetry/opentelemetry-collector#13044 (configoptional module) - open-telemetry/opentelemetry-collector#13119 (otlpreceiver migration) - open-telemetry/opentelemetry-collector#13252 (configgrpc TLS migration)
4 participants
Description
Uses
configoptional.Optionalfor fields inprotocolssection.Removes
Unmarshalmethod since it is no longer needed.These are both breaking changes, I think it would be a bit difficult to do this in two steps, I am happy to work on fixing contrib after this is merged.
Link to tracking issue
Fixes #12980