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.

Sign up for GitHub

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

config: Parse model from JSON #4806

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

config: Parse model from JSON #4806

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a71753c
implements parsing of a JSON configuration file into an OpenTelemetry…
carrbs Jan 13, 2024
4d06180
update CHANGELOG
carrbs Jan 13, 2024
0f4011d
fix formatting in CHANGELOG
carrbs Jan 13, 2024
d6381c7
Replace io.Reader with []byte to leverage json.Unmarshal()
carrbs Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

- Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108)
- Add `SDK.Shutdown` method in `"go.opentelemetry.io/contrib/config"`. (#4583)
- Add `ParseJSON` function to `"go.opentelemetry.io/contrib/config"` for parsing a JSON configuration file into an OpenTelemetryConfiguration. (#4412)

### Changed

Expand Down
13 changes: 12 additions & 1 deletion config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ package config // import "go.opentelemetry.io/contrib/config"

import (
"context"
"encoding/json"
"errors"
"io"

"go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/trace"
Expand Down Expand Up @@ -95,9 +97,18 @@ func WithOpenTelemetryConfiguration(cfg OpenTelemetryConfiguration) Configuratio
})
}

// ParseJSON parses a JSON configuration file into an OpenTelemetryConfiguration.
func ParseJSON(r io.Reader) (*OpenTelemetryConfiguration, error) {
var cfg OpenTelemetryConfiguration
decoder := json.NewDecoder(r)
if err := decoder.Decode(&cfg); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make some more advanced test? I am not sure if it works "out of the box". We may be missing JSON tags. See:

opentelemetry-go-contrib/Makefile

Line 322 in ba19074

--tags mapstructure \

@codeboten Am I correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pellared / @codeboten, I wasn't super clear on what the expected input would be, so perhaps I've overlooked something. I'd thought the OpenTelemetryConfiguration was specifically designed to build the parameters as mapstructures so the user could choose to define their config using either yaml or json. I (possibly naively) was building this to accept any valid json that had the required property from the configuration input used in the Makefile:

opentelemetry-go-contrib/Makefile

Line 324 in ba19074

${OPENTELEMETRY_CONFIGURATION_JSONSCHEMA_SRC_DIR}/schema/opentelemetry_configuration.json

Perhaps we can clarify what are the expected valid inputs for this (and #4373), and what we'd expect as output on valid/invalid cases? I feel this would help with implementation and writing more comprehensive tests. lmkwyt!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of removing this line:

opentelemetry-go-contrib/Makefile

Line 322 in ba19074

--tags mapstructure \

As then the OpenTelemetryConfiguration type will have mapstructure, json and yaml tags. See:

https://github.com/omissis/go-jsonschema/blob/6fcc83b8eebe874eeebc20474775280877cc62fe/main.go#L167-L168

Then we could simply provide examples how one can parse input to OpenTelemetryConfiguration using different libraries.

The parsing method do NOT need to do any validation e.g. check for required fields. The validation has to be done by NewSDK because the user could even create OpenTelemetryConfiguration "by hand" without parsing any JSON or YAML.

return nil, err
}
return &cfg, nil
}

// TODO: implement parsing functionality:
// - https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4373
// - https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4412

// TODO: create SDK from the model:
// - https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4371
43 changes: 43 additions & 0 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ package config

import (
"context"
"fmt"
"io"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -51,3 +54,43 @@ func TestNewSDK(t *testing.T) {
require.Equal(t, tt.wantShutdownErr, sdk.Shutdown(context.Background()))
}
}

func TestParseJSON(t *testing.T) {
tests := []struct {
name string
input string
wantErr error
wantType interface{}
}{
{
name: "valid JSON",
input: `{"file_format": "json", "disabled": false}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would return the same result

Suggested change
input: `{"file_format": "json", "disabled": false}`,
input: `{"file_format": "json", "disabled": false}THIS IS INTENTIONALLY MALFORMED`,

Reference: https://dev.to/taqkarim/you-might-not-be-using-json-decoder-correctly-in-golang-12mb

Maybe it would be safer to change the API to accept []byte instead of io.Reader and use json.Unmarshal? Especially that the performance difference is not big. Reference: https://dev.to/jpoly1219/to-unmarshal-or-to-decode-json-processing-in-go-explained-5870

Or maybe we should even not add such function, but simply add an an example to docs instead?

E.g.

func ExampleParseJSON() {
	input := `{"file_format": "0.1", "disabled": true}`

	var cfg config.OpenTelemetryConfiguration
	if err := json.Unmarshal(([]byte)(input), &cfg); err != nil {
		panic(err)
	}

	fmt.Printf("Disabled: %v", cfg.Disabled)

	// Output: Disabled: true
}

I am leaning towards adding a runnable example in docs instead of adding a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A runnable example in the docs sounds just fine to me. I can also rewrite this to accept the []byte string so we can use the more dependable json.Unmarshal method in the parser if we decide to take this route.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaning towards a runnable a example to reduce the API surface.

When I created #4412 I did NOT want OpenTelemetryConfiguration to have mapstructure tags, but after some time and disucssion agreed that it is acceptable. I will update the issue.

CC @codeboten

wantErr: nil,
wantType: &OpenTelemetryConfiguration{},
},
{
name: "invalid JSON",
input: `{"file_format": "json", "disabled":`,
wantErr: io.ErrUnexpectedEOF,
wantType: nil,
},
{
name: "missing required field",
input: `{"foo": "bar"}`,
wantErr: fmt.Errorf("field file_format in OpenTelemetryConfiguration: required"),
wantType: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := strings.NewReader(tt.input)
got, err := ParseJSON(r)
if err != nil {
require.Equal(t, tt.wantErr.Error(), err.Error())
} else {
assert.IsType(t, tt.wantType, got)
}
})
}
}