Fix default internal telemetry endpoint for collector #3744
Conversation
shuheiktgw
force-pushed
the
shuheiktgw/fix_default_metrics_configuration
branch
from
7eaf842 to
5677178
Compare
shuheiktgw
force-pushed
the
shuheiktgw/fix_default_metrics_configuration
branch
from
5677178 to
c0cc926
Compare
| return nil | ||
| } | ||
| // NOTE: Merge without overwrite. If a telemetry endpoint is specified, the defaulting | ||
| // respects the configuration and returns an equal value. | ||
| if err := mergo.Merge(s.Telemetry, tm); err != nil { |
There was a problem hiding this comment.
I couldn't just use mergo.Merge here because it doesn't work when an element exists in the readers. It simply doesn't add the default endpoint in that case.
| @@ -547,7 +548,9 @@ func TestCollectorDefaultingWebhook(t *testing.T) { | |||
| assert.NoError(t, test.expected.Spec.Config.Service.ApplyDefaults(logr.Discard()), "could not apply defaults") | |||
| } | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, test.expected, test.otelcol) | |||
| if diff := cmp.Diff(test.expected, test.otelcol); diff != "" { | |||
There was a problem hiding this comment.
I’d like to use cmp.Diff instead of assert.Equal because debugging was difficult. assert.Equal doesn’t show a detailed enough diff and omits some parts if the object is too deep.
There was a problem hiding this comment.
Can you add this as a comment to the code? I think it makes sense for future readers.
| return fmt.Errorf("telemetry config merge failed: %w", err) | ||
|
|
||
| var found bool | ||
| for _, reader := range readersSlice { |
There was a problem hiding this comment.
I've been preferring approaches where you use mapstructure to deserialize into a known struct instead of this map check / type assertion stuff. example
I think we should do a similar approach for internal telemetry especially as setting will only get more advanced going forward.
There was a problem hiding this comment.
Thank you for your feedback! I'm not entirely sure I understand your suggestion, so let me check if I got it right.
Are you suggesting defining a complete struct for the Telemetry field and decoding it using mapstructure.Decode? If so, I personally feel that instead of decoding it with mapstructure, we should change the type of the Telemetry field from AnyConfig to the newly defined struct.
Or are you suggesting that we use the newly defined struct to read the internal telemetry configuration and set default values dynamically if they are missing? In that case, if I understand correctly, we would still need to perform type assertions like we do now, since we can't set default values without confirming the types.
Please let me know if I'm missing anything! 🙇
There was a problem hiding this comment.
I'm fine with this:
Are you suggesting defining a complete struct for the Telemetry field and decoding it using mapstructure.Decode? If so, I personally feel that instead of decoding it with mapstructure, we should change the type of the Telemetry field from AnyConfig to the newly defined struct.
There was a problem hiding this comment.
Initially we should stick with anyconfig, but we should be able to convert that anyconfig into the defined struct from go-contrib i.e. this file. If not, we can define a smaller intermediate type and convert to that. I want to avoid any _, ok := ... as much as we can.
Description:
I've fixed the default internal telemetry endpoint for collector to use the new configuration.
Link to tracking Issue(s):
Testing:
Documentation: