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

[otelcol] Obtain the Collector's effective configuration from otelcol.Config #10139

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evan-bradley
Copy link
Contributor

Description

The current ConfmapProvider interface accurately reports the config provided to the Collector by the user, but fails to effectively report the Collector's effective configuration. In particular, it misses:

  • Default values for fields in Config structs.
  • Transformations done to Config structs by their Unmarshal or Validate methods.
  • Custom marshaling of types after we know the type of the config. This is most obvious with configopaque.String, where we want these values to always be redacted when sent out of the Collector.

As a result, I think we should attempt to get the Collector's effective configuration from otelcol.Config instead of using the map compiled by the confmap.Resolver. I initially intended to generate a confmap.Conf from otelcol.Config and call yaml.Marshal on that, but this encounters errors such as being unable to marshal Prometheus config which has invalid zero values. These errors don't occur when calling yaml.Marshal on otelcol.Config directly. I've updated and tested updating the ConfigWatcher interface to just accept an opaque any-typed data object and marshal that, and the end-to-end flow works. I haven't dug into this far enough to fully understand the differences between unmarshaling each type.

Two related PRs:

@evan-bradley evan-bradley requested a review from a team as a code owner May 10, 2024 21:21
@evan-bradley evan-bradley marked this pull request as draft May 10, 2024 21:21
@evan-bradley
Copy link
Contributor Author

To demonstrate the issue I'm seeing, this will fail:

cfg, _ := col.configProvider.Get(ctx, factories)
conf := confmap.New()
_ = conf.Marshal(cfg)
// This fails
bytes, err := yaml.Marshal(conf.ToStringMap())

This will succeed:

cfg, _ := col.configProvider.Get(ctx, factories)
// This succeeds
bytes, err := yaml.Marshal(cfg)

I'm fine just letting ConfigWatcher.NotifyConfig take an opaque any-typed parameter and telling the user to just marshal it with whatever format they want, but I don't fully understand why the first one fails. The second one also appears to make us more dependent on yaml since we don't have mapstructure as a buffer (e.g. we need to add yaml:"-" to confighttp.ClientConfig for the second snippet to work).

@sfc-gh-bdrutu
Copy link

As a result, I think we should attempt to get the Collector's effective configuration from otelcol.Config instead of using the map compiled by the confmap.Resolver. I initially intended to generate a confmap.Conf from otelcol.Config and call yaml.Marshal on that, but this encounters errors such as being unable to marshal Prometheus config which has invalid zero values. These errors don't occur when calling yaml.Marshal on otelcol.Config directly. I've updated and tested updating the ConfigWatcher interface to just accept an opaque any-typed data object and marshal that, and the end-to-end flow works. I haven't dug into this far enough to fully understand the differences between unmarshaling each type.

I think you should look into fixing marshal. /cc @atoulme

@evan-bradley
Copy link
Contributor Author

I think you should look into fixing marshal.

Thanks for the tip, I think that's also the best path forward. I found that the issue is that we don't do anything with yaml tags, which is what most third-party structs annotate on their fields. I will issue a PR to find a solution to that problem then update this PR afterward.

Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 91.89%. Comparing base (d9dbfbc) to head (84bf8b6).
Report is 1 commits behind head on main.

Files Patch % Lines
otelcol/collector.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10139      +/-   ##
==========================================
- Coverage   91.90%   91.89%   -0.01%     
==========================================
  Files         361      361              
  Lines       16970    16962       -8     
==========================================
- Hits        15596    15588       -8     
  Misses       1032     1032              
  Partials      342      342              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djaglowski
Copy link
Member

I think you should look into fixing marshal.

Thanks for the tip, I think that's also the best path forward. I found that the issue is that we don't do anything with yaml tags, which is what most third-party structs annotate on their fields. I will issue a PR to find a solution to that problem then update this PR afterward.

I have a possible solution in #10282.

Basically it adds a hook which looks for structs that have yaml tags but no mapstructure tags. For such structs, it uses the yaml package to "natively" convert the struct into map[string]any, which can then be marshaled by mapstructure in a generic way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants