[chore] Schema Processor Revamp [Part 4] - Manager/Provider #37402
Conversation
ankitpatel96
changed the title
ankitpatel96
changed the title
ankitpatel96
changed the title
ankitpatel96
changed the title
ankitpatel96
changed the title
ankitpatel96
force-pushed
the
ankit-schema-processor-4-manager-provider
branch
from
b846651 to
2cf50a3
Compare
|
Let me know once #37403 is merged and this is rebased, I will take a look. |
ankitpatel96
force-pushed
the
ankit-schema-processor-4-manager-provider
branch
from
2cf50a3 to
5fe87d3
Compare
| // of schema, the options allow for additional properties to be | ||
| // added to manager to enable additional locations of where to check | ||
| // for translations file. | ||
| func NewManager(targets []string, log *zap.Logger) (Manager, error) { |
There was a problem hiding this comment.
What are the targets? Are the they schema URLs? If yes perhaps targetShemaURLs as a name?
| // NewManager creates a manager that will allow for management | ||
| // of schema, the options allow for additional properties to be | ||
| // added to manager to enable additional locations of where to check | ||
| // for translations file. | ||
| func NewManager(targets []string, log *zap.Logger) (Manager, error) { |
| m.log.Debug("No valid schema url was provided, using no-op schema", | ||
| zap.String("schema-url", schemaURL), | ||
| ) |
| m.log.Debug("Not a known target, providing Nop Translation", | ||
| zap.String("schema-url", schemaURL), | ||
| ) |
There was a problem hiding this comment.
I feel this is critical feedback to the user , should be error
| } | ||
| m.rw.Lock() | ||
| m.providers = append(m.providers[:0], providers...) | ||
| m.rw.Unlock() |
| type testProvider struct { | ||
| fs *embed.FS | ||
| } | ||
|
|
||
| func NewTestProvider(fs *embed.FS) Provider { | ||
| return &testProvider{fs: fs} | ||
| } | ||
|
|
||
| func (tp testProvider) Lookup(_ context.Context, schemaURL string) (io.Reader, error) { | ||
| parsedPath, err := url.Parse(schemaURL) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| f, err := tp.fs.Open(parsedPath.Path[1:]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return f, nil | ||
| } |
There was a problem hiding this comment.
Why is test provider present here ?
| type Provider interface { | ||
| // Lookup whill check the underlying provider to see if content exists | ||
| // for the provided schemaURL, in the even that it doesn't an error is returned. | ||
| Lookup(ctx context.Context, schemaURL string) (content io.Reader, err error) |
There was a problem hiding this comment.
Can we rename this to Retrieve (tldr , make it consistent with confmap provider https://github.com/open-telemetry/opentelemetry-collector/blob/83d93cd7cf86b00ef6783365c127ebe4b33f06af/confmap/provider.go#L74C2-L74C10 )
There was a problem hiding this comment.
Can we return a string or a struct with result embedded
…r-4-manager-provider
…r-4-manager-provider
|
|
||
| targetTranslation, match := m.match[family] | ||
| if !match { | ||
| m.log.Warn("Not a known targetTranslation, providing Nop Translation", |
There was a problem hiding this comment.
The message seems incorrect, we don't return Nop Translation.
| func (m *manager) RequestTranslation(ctx context.Context, schemaURL string) (Translation, error) { | ||
| family, version, err := GetFamilyAndVersion(schemaURL) | ||
| if err != nil { | ||
| m.log.Error("No valid schema url was provided, using no-op schema", |
There was a problem hiding this comment.
The message is unclear, what does "using no-op schema" mean?
| cooldown time.Duration | ||
| callcount int | ||
| limit int |
There was a problem hiding this comment.
It would be nice to doc-comment these fields to explain how they work together to do rate limiting (plus resetTime, I think?).
|
The build failed. Appears to be an unrelated test, but please double check. |
|
Prometheus tests have been flakey, will merge after successful build. |
…r-4-manager-provider
…m:ankitpatel96/opentelemetry-collector-contrib into ankit-schema-processor-4-manager-provider
Description
This is a slice of changes from #35248
It's stacked on top of #37403
This PR details the Manager and Provider. The Manager is responsible for fetching and caching the translations. The Provider helps fetch the schema URLs.
It's a pretty small PR compared to part 3 -
❯ git diff ankit-schema-processor-3-translation --stat
.../translation/manager.go | 131 ++++++++++
.../translation/manager_test.go | 68 +++++
.../translation/provider.go | 73 ++++++
.../translation/provider_test.go | 52 ++++
.../testdata/schema.yaml | 112 ++++++++
5 files changed, 436 insertions(+)
Testing
Unit tests