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

[confmap] Allow using any type as a string #10800

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Aug 5, 2024

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:
exporters:
  otlp:
    endpoint: http://${env:HOST}/api/v1/traces
  1. Pass really big numbers as strings (e.g. 10000000000000000000)

  2. Pass null as a string.

Types that can be returned by our current YAML parser

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:

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)
		}
	})	
}

@mx-psi mx-psi force-pushed the mx-psi/add-string-representation-for-everything branch from de0305a to f667b9c Compare August 5, 2024 11:37
@mx-psi mx-psi force-pushed the mx-psi/add-string-representation-for-everything branch from f667b9c to 9c94725 Compare August 5, 2024 11:39
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.71%. Comparing base (7c2002a) to head (b2e8804).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10800      +/-   ##
==========================================
+ Coverage   91.70%   91.71%   +0.01%     
==========================================
  Files         406      406              
  Lines       18947    18947              
==========================================
+ Hits        17376    17378       +2     
+ Misses       1213     1209       -4     
- Partials      358      360       +2     

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

@mx-psi mx-psi marked this pull request as ready for review August 5, 2024 12:00
@mx-psi mx-psi requested review from a team and dmitryax August 5, 2024 12:00
@mx-psi mx-psi requested a review from TylerHelmuth August 5, 2024 12:15
@mx-psi mx-psi merged commit 93cbae4 into open-telemetry:main Aug 5, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 5, 2024
mx-psi added a commit that referenced this pull request Aug 7, 2024
<!--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 -->

Shows that #10795 (as well as #10800) fixed this issue.

#### Link to tracking issue

Fixes #10799
dmitryax pushed a commit that referenced this pull request Sep 27, 2024
…ts (#11241)

#### Description

This addresses
#10659.
In #10800
we removed restrictions on the types that can be allowed if expanded but
in our case, this still fails because of the checks existing in
`provider.checkRawConfType()`

This adds `time.Time` as a type in the `provider.checkRawConfType()`
instead of removing it completely.

#### Link to tracking issue
Fixes
#10659

<!--Describe what testing was performed and which tests were added.-->
#### Testing
- added a test case to handle time formats in provider.go and expand.go
- added an e2e test case to handle time formats
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this pull request Oct 8, 2024
…ts (open-telemetry#11241)

#### Description

This addresses
open-telemetry#10659.
In open-telemetry#10800
we removed restrictions on the types that can be allowed if expanded but
in our case, this still fails because of the checks existing in
`provider.checkRawConfType()`

This adds `time.Time` as a type in the `provider.checkRawConfType()`
instead of removing it completely.

#### Link to tracking issue
Fixes
open-telemetry#10659

<!--Describe what testing was performed and which tests were added.-->
#### Testing
- added a test case to handle time formats in provider.go and expand.go
- added an e2e test case to handle time formats
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this pull request Dec 19, 2024
…ts (open-telemetry#11241)

#### Description

This addresses
open-telemetry#10659.
In open-telemetry#10800
we removed restrictions on the types that can be allowed if expanded but
in our case, this still fails because of the checks existing in
`provider.checkRawConfType()`

This adds `time.Time` as a type in the `provider.checkRawConfType()`
instead of removing it completely.

#### Link to tracking issue
Fixes
open-telemetry#10659

<!--Describe what testing was performed and which tests were added.-->
#### Testing
- added a test case to handle time formats in provider.go and expand.go
- added an e2e test case to handle time formats
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.

3 participants