Generate the batch processor config from schema #13155
Conversation
| @@ -1,70 +1,74 @@ | |||
| // Copyright The OpenTelemetry Authors | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
| // Code generated by github.com/atombender/go-jsonschema, DO NOT EDIT. | |||
There was a problem hiding this comment.
Is it a problem that the file doesn't have this header:
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0
There was a problem hiding this comment.
Do you need me to add more tests now for use cases not covered by the batch processor? Or are you ok with adding these gradually in subsequent PRs?
| @@ -200,3 +200,5 @@ telemetry: | |||
| # Optional: array of attributes that were defined in the attributes section that are emitted by this metric. | |||
| # Note: Only the following attribute types are supported: <string|int|double|bool> | |||
| attributes: [string] | |||
|
|
|||
| # TODO: Add the new "config" field here too. | |||
There was a problem hiding this comment.
I'll need to add schema for the "config" field before opening the PR for reviews.
| // When this is set to zero, batched data will be sent immediately. | ||
| Timeout time.Duration `mapstructure:"timeout"` | ||
| // Prevent unkeyed literal initialization. | ||
| _ struct{} `mapstructure:"_"` |
There was a problem hiding this comment.
It would be better if this doesn't have mapstructure:"_" struct tags. I could try to add a feature to go-jsonschema which disables them on a per-attribute level. Something like disableStructTags:
_:
description: >-
Prevent unkeyed literal initialization.
type: integer
goJSONSchema:
identifier: _
type: struct{}
nillable: true
disableStructTags: true| if v, ok := raw["send_batch_max_size"]; !ok || v == nil { | ||
| plain.SendBatchMaxSize = 0.0 | ||
| } | ||
| if v, ok := raw["send_batch_size"]; !ok || v == nil { |
There was a problem hiding this comment.
Since the schema contains minimum: 0, normally go-jsonschema would also generate code like this:
if 0 > plain.SendBatchSize {
return fmt.Errorf("field %s: must be >= %v", "send_batch_size", 0)
}
It doesn't do it here because he overrode the type to be uint32. It's not a problem right now, but it might be a problem if we wanted the minimum to be larger than 0. I should try to fix this upstream.
| var _ component.Config = (*Config)(nil) | ||
|
|
||
| // Validate checks if the processor configuration is valid | ||
| func (cfg *Config) Validate() error { |
There was a problem hiding this comment.
To leverage go-jsonschema's validation abilities, it would be nice to marshal to json and unmarshal. Then we could see if boundaries checks such minimum and maximum for integers work ok. But to do this, I suppose we need json struct tags. Would that be ok? Otherwise we'd need a new feature in jsonschema to handle this without relying on json, which would be useful for mapstructure users.
| MetadataCardinalityLimit: defaultMetadataCardinalityLimit, | ||
| } | ||
| cfg := &Config{} | ||
| json.Unmarshal([]byte("{}"), cfg) // Unmarshal empty JSON to get default values |
There was a problem hiding this comment.
There's no need to have json struct tags if all we need is to fill in the defaults.
|
Hi, thanks for the mention. Just a word of advice: some of the PRs I merged to add support for this project caused some BC breaks that I am going to address in the next release (v0.21.0) and that will revert some of the behaviors to the previous version of the library. the new behaviors will need to be explicitly activated: this will probably break your code, but it'll just be a matter of activating a flag to fix it. please use main if you want to incorporate those changes early on |
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (62.19%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #13155 +/- ##
==========================================
- Coverage 91.27% 91.09% -0.18%
==========================================
Files 508 510 +2
Lines 28736 28883 +147
==========================================
+ Hits 26228 26312 +84
- Misses 1992 2043 +51
- Partials 516 528 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This is following up on #10694. Unlike the previous PR, this one is much smaller because it only adds schema for the batch processor. Also, thanks to @omissis's amazing support go-jsonschema has a lot more features now and we might be able to just use the upstream go-jsonschema without a need for a temporary fork 🙏
I'm hoping that we can implement this for the batch processor first, and then gradually expand into more and more components. If for some reason the autogeneration becomes an issue, it can be disabled by commenting it out in the
metadata.yamlfile and editingconfig.gomanually.The PR is still in draft stage because there are a few TODOs for which I need feedback from maintainers.
Link to tracking issue
Fixes #9769
Testing
There is a test in mdatagen, but the things it tests are limited to the functionality required for the batch processor.
Documentation
I only edited the mdatagen readme file.