Skip to content
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

config: proposal: Do not use pointers in model #4372

Closed
pellared opened this issue Oct 2, 2023 · 1 comment
Closed

config: proposal: Do not use pointers in model #4372

pellared opened this issue Oct 2, 2023 · 1 comment
Labels
area: config Related to config functionality

Comments

@pellared
Copy link
Member

pellared commented Oct 2, 2023

I find having pointers in domain models not clean.

From the usage perspective it often leads to creating helpers like ptrInt, ptrString (related Go proposal golang/go#45624)

We could use (and define) something like this instead of using pointers:

package schema

type Optional[T any] struct {
  value T
  empty bool
}

func OptionalValue[T any](T v) {
  return Optional{value: v}
}

func OptionalEmpty[T any](T v) {
  return Optional{empty: true}
}

func (o Optional[T]) Empty() bool {
  return o.empty
}

func (o Optional[T]) Value() T {
  if o.empty {
    panic("empty value")
  }
  return o.value
}

func (o Optional[T]) ValueOrDefault(dft T) T {
  if o.empty {
    return dft
  }
  return o.value
}

I think if we could use such generic type then the code would be more readable and maintainable. Also using such code should be safer as writing a nil deference panic would be harder. Moreover, if the average case is to pass the config model around a single call stack, non pointer is probably better performance-wise.

On the other hand using such generic would require more code if e.g. the user would like to log the config model value.

Based on discussion in #4228 (comment)

@pellared pellared added the area: config Related to config functionality label Oct 2, 2023
@pellared pellared changed the title config: Do not use pointers in model config: proposal: Do not use pointers in model Oct 2, 2023
@pellared
Copy link
Member Author

pellared commented Oct 2, 2023

More drawbacks:

  1. It does not feel idiomatic in go to use such generics. Also @MadVikingGod pointed out that shouldn't make a helper type just to avoid some helper functions. The proposal: expression to create pointer to simple types golang/go#45624 should address the issue
  2. How we would using pointers is easier if one would like to e.g. not create a meter provider
cfg, err := configyaml.Parse(file)
// err handling
cfg.Metrics = nil // do not create a meter provider
sdk, err := config.NewSDK(cfg)
  1. Usually the config would be created by parsing configuration files. We do not expect OTel users to create them by hand (the config model should easily created by the parser modules).

Closing as I also changed my mind regarding my proposal.

CC @codeboten

@pellared pellared closed this as completed Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: config Related to config functionality
Projects
None yet
Development

No branches or pull requests

1 participant