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

Flatten schema pkg #4538

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,22 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Added

- The `Parse` and `ParseFile` functions are added to `go.opentelemetry.io/otel/schema` to replace the ones formerly found in `go.opentelemetry.io/otel/schema/v1.0` and `go.opentelemetry.io/otel/schema/v1.1`. (#4538)
- The `Schema` and its related types are added to `go.opentelemetry.io/otel/schema` to replace the ones formerly found in `go.opentelemetry.io/otel/schema/v1.0` and `go.opentelemetry.io/otel/schema/v1.1`. (#4538)
- Add `WithEndpointURL` option to the `exporters/otlp/otlpmetric/otlpmetricgrpc`, `exporters/otlp/otlpmetric/otlpmetrichttp`, `exporters/otlp/otlptrace/otlptracegrpc` and `exporters/otlp/otlptrace/otlptracehttp` packages. (#4808)

### Fixed

- Fix `ContainerID` resource detection on systemd when cgroup path has a colon. (#4449)
- Fix `go.opentelemetry.io/otel/sdk/metric` to cache instruments to avoid leaking memory when the same instrument is created multiple times. (#4820)

### Removed

- The `go.opentelemetry.io/otel/schema/v1.0` package.
Use the similar functions and types added to `go.opentelemetry.io/otel/schema` instead. (#4538)
- The `go.opentelemetry.io/otel/schema/v1.1` package.
Use the equivalent functions and types added to `go.opentelemetry.io/otel/schema` instead. (#4538)

## [1.23.0-rc.1] 2024-01-18

This is a release candidate for the v1.23.0 release.
Expand Down
33 changes: 0 additions & 33 deletions schema/README.md

This file was deleted.

26 changes: 17 additions & 9 deletions schema/v1.0/types/types.go → schema/example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,24 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package types // import "go.opentelemetry.io/otel/schema/v1.0/types"
package schema

// TelemetryVersion is a version number key in the schema file (e.g. "1.7.0").
type TelemetryVersion string
import (
"fmt"
"log"
)

// SpanName is span name string.
type SpanName string
const path = "./testdata/valid-example.yaml"

// EventName is an event name string.
type EventName string
func ExampleParseFile() {
s, err := ParseFile(path)
if err != nil {
log.Fatal(err)
}

// MetricName is a metric name string.
type MetricName string
fmt.Println("file format:", s.FileFormat)
fmt.Println("schema URL:", s.SchemaURL)
// Output:
// file format: 1.1.0
// schema URL: https://opentelemetry.io/schemas/1.1.0
}
74 changes: 0 additions & 74 deletions schema/internal/parser_checks.go

This file was deleted.

41 changes: 0 additions & 41 deletions schema/internal/parser_checks_test.go

This file was deleted.

56 changes: 56 additions & 0 deletions schema/parser.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// Package schema provides types and utilities used to interact with
// [OpenTelemetry schema files].
//
// [OpenTelemetry schema files]: https://github.com/open-telemetry/opentelemetry-specification/blob/007f415120090972e22a90afd499640321f160f3/specification/schemas/file_format_v1.1.0.md
package schema // import "go.opentelemetry.io/otel/schema"

import (
"io"
"os"

"gopkg.in/yaml.v3"
)

// ParseFile parses a Schema from the schema file found at path.
func ParseFile(path string) (*Schema, error) {
file, err := os.Open(path)
if err != nil {
return nil, err
}

Check warning on line 33 in schema/parser.go

View check run for this annotation

Codecov / codecov/patch

schema/parser.go#L32-L33

Added lines #L32 - L33 were not covered by tests
return Parse(file)
}

// Parse parses a Schema read from r.
//
// If r contains a Schema with a file format version outside FileFormatRange,
// an error will be returned.
//
// If r contains an invalid schema URL an error will be returned.
func Parse(r io.Reader) (*Schema, error) {
d := yaml.NewDecoder(r)
d.KnownFields(true)
Copy link
Member

@pellared pellared Sep 22, 2023

Choose a reason for hiding this comment

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

Putting one long comment with many remarks and thoughts as I find them very related to each other.

I have some problems with the parsing logic and interpreting the specification.

Extensibility?

From stable spec:

the schema file format is extensible.

What does it mean? Can we add custom fields that may be added in future versions to forward compatibility? Should consider removing this line? On the other hand, the author maybe just wanted to say that the file_format may evolve in future version (and the "extensibility" term is not used correctly/precisely). Extensibility in "formats" usually means that custom things can be added thanks to some extension points e.g. https://www.w3schools.com/xml/el_extension.asp. As far as I see our current implementation uses d.KnownFields(true). It errors when encounters unknown fields for the currently supported version. Our implementation does not offer forward compatibility. In my opinion, our implementation is OK and the problem is that the specification is not clear. Since it lacks normative wording, we may leave it as is.

Parsing logic vs file_format version

I noticed that our implementation will NOT throw an error when the file has file_format: 1.0.0, but uses a field introduced from 1.1.0 version. For example, if you change file_format to 1.0.0 in valid-example.yaml the ParseFile will successfully parse without any problem. Take notice that, the experimental (sic!) file format says:

Defines the file format. MUST be set to 1.1.0.

Therefore, I am not sure if the current implementation complies with the specification.

If I understand correctly, if we would like to be so strict, we would need to have dedicated structs for each minor version. I find it not pragmatic (an overkill). I would NOT like doing it and I would rather change the specification (if needed) than our implementation.

For reference let me try to describe the parsing strategy that browsers and Docker Compose have for parser HTML/YAML files.

  1. Parser just interprets the "major" version. The minor version specified by the user is "ignored" by the user. It is only informative for humans (what is more, the latest Docker version even ignores the version field and uses only the latest version of the schema when parsing).
  2. Parser emit warnings (errors that can be ignored) in case of unknown fields that it does not understand. However, it should still return the parsed object (forward compatible). The parser could also silently ignore unknown fields.

In my opinion, the current implementation is OK. Right now we simply parse against the latest supported version. We can decide later what we will do when v2 of file_version is defined.

Possible safe forward compatibility

If r contains a Schema with a file format version higher than FileFormat, an error will be returned.

We could still parse a schema if it does NOT use any new field (a kind of forward compatibility which offers support until does NOT use unknown features). I think we could offer this later as this would not be a breaking change. I would view it as an enhancement. However, it is YAGNI we will have to bump the file_version support as soon as new OTel semconv schema uses the file_version.

Summary

I like the current design and implementation. However, it would be good to ensure that the parsing implementation complies with the specification. Especially the part that we are able to parse files which have file_format: 1.0.0 while also having syntax from a newer format e.g. 1.1.0. In my opinion, the current implementation is OK. I believe it would be more practical to clarify or modify the specification instead of altering the proposed implementation.

It also worth to notice that any validation logic changes will affect https://github.com/open-telemetry/build-tools/tree/main/schemas. As far as I reviewed, the current PR does NOT introduce any changes in the parsing+validation logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had considered restricting parsing to only allow parsing of a specific version of the file format, but I don't think that's the user experience we want to provide. If a user has a schema that incorrectly states it is 1.0.0 and uses 1.1.0 fields, I would rather produce a valid schema for the user. I doubt they care that the schema was incorrect, they just want the data.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Should we add tests for this behavior and should it be documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Should we add tests for this behavior and should it be documented?

Yeah, that sounds good to me. I'll update in a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is another section in the file format that specifies that this is the intended behavior:

Correspondingly:

Consumers of the schema file SHOULD NOT attempt to interpret the schema file if the MAJOR version number is different (higher or lower) than what the consumer supports.

Consumers of the schema file SHOULD NOT attempt to interpret the schema file if the MINOR version number is higher than what the consumer supports.

Consumers MAY ignore the PATCH number.


var s Schema
if err := d.Decode(&s); err != nil {
return nil, err
}

if err := s.validate(); err != nil {
return nil, err
}

Check warning on line 54 in schema/parser.go

View check run for this annotation

Codecov / codecov/patch

schema/parser.go#L53-L54

Added lines #L53 - L54 were not covered by tests
return &s, nil
}
Loading
Loading