<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes#13308
### Summary
This PR replaces the YAML parsing library `sigs.k8s.io/yaml` with
`go.yaml.in/yaml`.
---------
Signed-off-by: 7h3-3mp7y-m4n <emailtorash@gmail.com>
Adds checkapi to the tools and a make target to run it.
Fixes#12360
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
#### Description
This updates replace gopkg.in/yaml.v3 with sigs.k8s.io/yaml
#### Link to tracking issue
Fixes#12827
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
<!-- Issue number if applicable -->
Add error hint field to surface YAML parsing errors. This only surfaces
the error for top-level URIs but it's a step in the right direction.
Error when passing a file with contents `[invalid:,` as the config:
Before:
```
retrieved value (type=string) cannot be used as a Conf
```
After:
```
retrieved value (type=string) cannot be used as a Conf: assuming string type since contents are not valid YAML: yaml: line 1: did not find expected node content
```
#### Link to tracking issue
Updates #12000
<!--Describe what testing was performed and which tests were added.-->
#### Testing
<!--Describe the documentation added.-->
#### Documentation
<!--Please delete paragraphs that you did not use before submitting.-->
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
I tried to improve the docs on watchers based on the issue below
<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes#12115
<!--Describe the documentation added.-->
#### Documentation
Clarified some code comments that were incorrect (there's no
Retrieved.Get method) and included a new code sample for a Provider that
uses the watcher func
<!--Please delete paragraphs that you did not use before submitting.-->
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
<!-- Issue number if applicable -->
Marks `confmap.strictlyTypedInput` as stable.
#### Link to tracking issue
Fixes#10552
Blocked by:
- #10794
- #10795
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Allows any type to be used as a string if the target field is a string
or the value is expanded in inline position. Inspired by #10794. This is
not a breaking change (previously we would return an error for these).
Some things that you can do after this PR that you couldn't do before
it:
1. Set `HOST` to `[2001:db8::8a2e:370:7334]` and use it in an endpoint:
```yaml
exporters:
otlp:
endpoint: http://${env:HOST}/api/v1/traces
```
2. Pass really big numbers as strings (e.g. `10000000000000000000`)
3. Pass `null` as a string.
<details>
<summary>Types that can be returned by our current YAML parser</summary>
Our current way of using the YAML parser has these types: `string`,
`nil`, `int`, `uint64`, `float64`, `map[any]any`, `map[string]any`,
`[]any`.
There is no documentation for this, but the following fuzzing test did
not find any failing cases after 20 minutes of continous run:
```go
package main
import (
"testing"
"gopkg.in/yaml.v3"
)
func FuzzTest(f *testing.F) {
f.Fuzz(func(t *testing.T, data []byte){
var b any
err := yaml.Unmarshal([]byte(data), &b)
if err != nil {
return
}
switch b.(type) {
case string, nil, int, uint64, float64, map[any]any, map[string]any, []any:
return
default:
t.Errorf("Unexpected type %T", b)
}
})
}
```
</details>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
<!-- Issue number if applicable -->
If YAML parsing fails, assume the user wanted to pass the value as a
string.
This has the downside that the error messages are less informative: it
will tell you it expected something other than a string instead of the
YAML parser error.
A future improvement could be to pass these errors down as extra
metadata up until the unmarshaling stage.
#### Link to tracking issue
Fixes#10759
<!--Describe what testing was performed and which tests were added.-->
#### Testing
<!--Describe the documentation added.-->
Added test case for this.
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
<!-- Issue number if applicable -->
- Adds new `expandedValue` struct that holds the original string
representation if available for values resolved from a provider.
- Removes any mention of `expandedValue` in the public API by adding a
`sanitize` step before returning any `Get`s or `ToStringMap`s.
- Adds new decoding hook that checks if the target field is of `string`
type and uses the string representation in that case.
#### Link to tracking issue
Fixes#10605, Fixes#10405, Fixes#10659
<!--Describe what testing was performed and which tests were added.-->
#### Testing
<!--Describe the documentation added.-->
This changes the behavior in some cases, I update the test cases.
#### Documentation
<!--Please delete paragraphs that you did not use before submitting.-->
| ENV value | ${ENV} before unification | ${ENV} in v0.105.0 (also
${env:ENV} before unification) | Value after this PR |
|----------------------------|----------------------------|---------------------------------------------------------|----------------------------|
| foo\nbar | foo\nbar | foo bar | foo\nbar |
| 1111:1111:1111:1111:1111:: | 1111:1111:1111:1111:1111:: | **Error** |
1111:1111:1111:1111:1111:: |
| "0123" | "0123" | 0123 | "0123" |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
<!-- Issue number if applicable -->
Allow using strings parseable in YAML as `map[string]any` in inline
position
#### Link to tracking issue
Relates to #10605 (does not fix it since it's not in inline position)
#### Description
<!-- Issue number if applicable -->
- Add `confmap.strictlyTypedInput` feature gate that introduces stricter
type checks when resolving configuration
- Make `confmap.NewRetrievedFromYAML` function public so that external
providers have consistent behavior when resolving YAML
- Adds `confmap.Retrieved.AsString` method to retrieve string
representation for retrieved values
#### Link to tracking issue
Relates to #9854, updates #8565, #9532
**Description:**
Follows
https://github.com/open-telemetry/opentelemetry-collector/pull/9443,
relates to
https://github.com/open-telemetry/opentelemetry-collector/pull/9513.
This builds on
https://github.com/open-telemetry/opentelemetry-collector/pull/9228 to
demonstrate the concept.
This shows one way of extending the otelcol APIs to allow passing
converters and providers from the builder with the new settings structs
for each type.
I think this approach has a few advantages:
1. This follows our pattern of passing in "factory" functions instead of
instances to the object that actually uses the instances.
2. Makes the API more declarative: the settings specify which modules to
instantiate and which settings to instantiate them with, but don't
require the caller to actually do this.
3. Compared to the current state, this allows us to update the config at
different layers. A distribution's `main.go` file can specify the
providers/converters it wants and leave the settings to be created by
`otelcol.Collector`.
The primary drawbacks I see here are:
1. This is a little more opinionated since you don't have access to the
converter/provider instances or control how they are instantiated. I
think this is acceptable and provides good encapsulation.
2. The scheme->provider map can now only be specified by the providers'
schemes, which is how it is currently done by default. I would want to
hear what use cases we see for more complex control here that
necessitates using schemes not specified by the providers.
cc @mx-psi
---------
Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
**Description:**
Creates a logger in the confmap.ProviderSettings and uses it to log when
there is a missing or blank environment variable referenced in config.
For now the noop logger is used everywhere except tests.
**Link to tracking Issue:**
[5615](https://github.com/open-telemetry/opentelemetry-collector/issues/5615)
**Testing:**
I wrote unit tests that ensured
1. logging occurred when an environment variable was unset
2. logging occcured when the env var was empty.
3. there was no log when an env var was used correctly
I also started the otel collector with the sample config - and added an
env var reference in the sample config. I then inserted a print
statement next to each log call to see whether my code paths were hit in
the live application. I then went through the 3 cases mentioned above
and ensured that logging behavior was accurate.
**Description:**
For both #5615 and #9162 we need to be able to log during the confmap
resolution.
My proposed solution is to pass a `*zap.Logger` to converters and
providers during initialization. These components can then use this to
log any warnings they need. This PR does the first step: being able to
pass anything to converters and providers during initialization.
The obvious alternative to this is to change the interface of
`confmap.Provider` and `confmap.Converter` to pass any warnings in an
explicit struct. I think the `*zap.Logger` alternative is more natural
for developers of providers and converters: you just use a logger like
everywhere else in the Collector.
One problem for the Collector usage of `confmap` is: How does one pass a
`*zap.Logger` before knowing how a `*zap.Logger` should be configured? I
think we can work around this by:
1. Passing a special 'deferred' Logger that just stores the warnings
without actually logging them (we can use something like
`zaptest/observer` for this)
2. Resolving configuration
3. Building a `*zap.Logger` with said configuration
4. Logging the entries stored in (1) with the logger from (3) (using
`zaptest/observer` we can do that by taking the `zapcore.Core` out of
the logger and manually writing)
**We don't actually need ProviderSettings today, just ConverterSettings,
but I think it can still be useful.**
**Link to tracking Issue:** Relates to #5615 and #9162
---------
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
* [chore] use license shortform
To remain consistent w/ contrib repo, see https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/22052
Signed-off-by: Alex Boten <aboten@lightstep.com>
* make goporto
Signed-off-by: Alex Boten <aboten@lightstep.com>
---------
Signed-off-by: Alex Boten <aboten@lightstep.com>
Currently was documented that the format should be compatible with the URI definition (see https://datatracker.ietf.org/doc/html/rfc3986),
but the scheme name restriction was not copied from the RFC, and no tests to enforce. This PR clearly documents the characters allowed in the scheme name and add confmaptest helper func to test for valid scheme names.
Signed-off-by: Bogdan <bogdandrutu@gmail.com>
This change makes implementations cleaner, since they can return `nil, err` in case of an error instead of a zero initialized Retrieved.
This is a breaking change, but there are very very few implementation of the Provider, so it should be safe to just break it.
Signed-off-by: Bogdan <bogdandrutu@gmail.com>
Adding the latest version of go to the tests run by CI. To pass the tests, the following changes were required:
- run make genpdata
- fix test certificates to address errors caused by the rejection of duplicate extensions in TLS handshakes
It is ok to change NewRetrievedFromMap to NewRetrieved since it was just moved, not yet released. This is a step towards supporting providing any value type.
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
7h3-3mp7y-m4n
Antoine Toulme
Alex Boten
Pablo Baeyens
Matthew Sainsbury
Iris Grace Endozo
Bogdan Drutu
Antoine Toulme
Evan Bradley
Ankit Patel
Alex Boten
Sean Marciniak