Change config.Retrieved to allow retrieving different types, map, string, etc.

[bogdandrutu]

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

[codecov]

Codecov Report

Merging #5198 (56b42b3) into main (9b078e9) will decrease coverage by 0.04%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main    #5198      +/-   ##
==========================================
- Coverage   90.60%   90.56%   -0.05%     
==========================================
  Files         187      188       +1     
  Lines       11061    11082      +21     
==========================================
+ Hits        10022    10036      +14     
- Misses        820      825       +5     
- Partials      219      221       +2     

Impacted Files	Coverage Δ
service/config_provider.go	89.74% <75.00%> (-2.50%)	⬇️
config/configtest/configtest.go	100.00% <100.00%> (ø)
config/mapprovider.go	100.00% <100.00%> (ø)
config/mapprovider/envmapprovider/mapprovider.go	100.00% <100.00%> (ø)
config/mapprovider/filemapprovider/mapprovider.go	84.21% <100.00%> (ø)
config/mapprovider/yamlmapprovider/mapprovider.go	100.00% <100.00%> (ø)
pdata/internal/common.go	94.16% <0.00%> (-0.78%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b078e9...56b42b3. Read the comment docs.

[bogdandrutu]

@mx-psi feedback is welcome. I think this change gives us flexibility in the future and probably we should do it anyway.

[mx-psi]

I will have a look tomorrow morning :) I saw this and it looks reasonable at a first glance, but I haven't had time to give it a proper look

[mx-psi]

This makes sense to me, I think my only objection is that it can be a bit cryptic to figure out how it works by just looking at the code, so we probably want more docs/explicitness.

The detail about the "" path in particular should be documented; I wonder if we should have a Path type alias with an RootPath = "" constant and make it MergeTo(dest *Map, destPath Path)

[bogdandrutu]

@mx-psi thanks for the feedback. I did revert the change to make Retrieved interface, since it was not backwards compatible (first deprecate all the fields and expose the funcs, then will remove the fields and can transform in interface). I also change the MoveTo to not accept a destination (since in lots of places we need to call this), and will add later a MoveToAt that accepts a destination as well and will document at that moment the behavior of the path (if we need).

[mx-psi]

LGTM as a first step

[bogdandrutu]

@mx-psi last question here, should we go with "close" as "RetrievedOption" because literally is optional here? so we will define a "Option" and we will have WithClose, instead of asking users to pass nil when no close needed?

[mx-psi]

Sounds better, yes, it's also how we pass the function pointers on the component helpers, so we already have the precedent there.

[bogdandrutu]

@mx-psi per the feedback from the expand PR (#4742), I changed this to "AsMap" instead of the merge. The problem with the "Merge" like patter, is that is impossible to merge (parse into a location) if the expanded value is part of an array, so in order to not have to suffer and limit the functionality to not work on arrays, I decided to go with the pattern where we return the value.

[mx-psi]

@mx-psi per the feedback from the expand PR (#4742), I changed this to "AsMap" instead of the merge. The problem with the "Merge" like patter, is that is impossible to merge (parse into a location) if the expanded value is part of an array, so in order to not have to suffer and limit the functionality to not work on arrays, I decided to go with the pattern where we return the value.

Retrieved would have a AsMap and... AsRaw methods then? Also, I don't think I understand now the arrays issue and it would help if you can share an example (here or in the other PR) about what the issue is.

[mx-psi]

Ah, I think I understand now, let me spell it out loud to confirm. Say we have:

receivers:
  otlp:
    protocols:
      grpc:

exporters:
  nop

processors:
  transform:
    traces:
      queries:
        -  ${env:MY_QUERY}
        - keep_keys(resource.attributes, "service.name", "service.namespace", "cloud.region")
        - set(name, attributes["http.route"])
service:
  pipelines:
    traces:
      receivers: [otlp]
      processors: [transform]
      exporters: [nop]

We then don't have a way to specify the path to the environment variable, we can't do something like processors::transform::queries::[0]. Is that what you are pointing at?

[bogdandrutu]

Indeed, unless we invent a path format like you proposed, we cannot reference that env variable in the array. We can also go that path, but I feel is too hacky

[mx-psi]

@bogdandrutu How does something like this (naming issues aside) sound?

type Segment struct {
  key string
  index int
}

func Key(key string) Segment {return Segment{key: key, index: -1}}
func Index(idx int) Segment {return Segment{key: "", index: idx}}

type Path struct{
  segments []string
}

func NewPath(segments ...Segment) Path {return Path{segments}}

and then have

func (r Retrieved) MergeToAt(dest *Map, path Path) { 
  // ...
}

We don't need to support Index now and just have the Key function for now. Something like this

1. would allow us to support it in the future and
2. would avoid having to return interface{}.

---

There is still the problem of something like:

processors:
  transform:
    traces:
      queries:
        - keep_keys(resource.attributes, ${env:SOME_ENV_VAR}, "service.namespace", "cloud.region")

which I am not sure how/if it will be supported, even if solving the array problem.

[bogdandrutu]

The last example is a substring in a string from what I understand. With the return of interface... We may be able to solve that, since we can check if it is a value that we can stringify and concat it. With the merge we need to have a way to say merge in this string between this index and that index

[bogdandrutu]

I am curious about this requirement would avoid having to return interface{}. we already return and accept interface{} in the Map (and all the similar concepts do the same). Why is that so important?

[mx-psi]

The last example is a substring in a string from what I understand.

Yes, that's it.

I am curious about this requirement would avoid having to return interface{}. we already return and accept interface{} in the Map (and all the similar concepts do the same). Why is that so important?

So this was mentioned by @Aneurysm9 on #4742 (comment). I agree that whenever possible avoiding interface{} is a good idea: having explicit types allows us (and potential future users) to catch some kind of errors at compile time instead of at runtime. I don't know if Anthony had some other benefit in mind.

I think the alternatives have been explored here and at #4742 at length and in my mind, unless the "substring in a string" case is handled in some special way, it seems like the only one that does not seem prohibitively complex or limited is returning an interface{} indeed.

[bogdandrutu]

having explicit types allows us (and potential future users) to catch some kind of errors at compile time instead of at runtime.

To be honest, because we need to unmarshal these values into the config, there is no way you will catch errors at compilation, unless I am missing something. Also the return value depends on external sources like what you have in zookeeper, voltdb, etc....

[mx-psi]

having explicit types allows us (and potential future users) to catch some kind of errors at compile time instead of at runtime.

To be honest, because we need to unmarshal these values into the config, there is no way you will catch errors at compilation, unless I am missing something. Also the return value depends on external sources like what you have in zookeeper, voltdb, etc....

That is a fair point. I still feel like it's generally better to have explicit types but because of our discussion above, I am happy with the interface{} solution since the alternatives seem too complex.

The only missing thing in my mind to merge this PR is to deal with the Close function as an option from #5198 (comment)

[bogdandrutu]

Will do that... Wanted to finish the debate about the API

[bogdandrutu]

@mx-psi done, PTAL one more time if you have time before your day ends :)

[mx-psi]

Looks good!