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

Support default configs for storage backends #5691

Merged
merged 6 commits into from
Jul 1, 2024

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Jul 1, 2024

Which problem is this PR solving?

  • Many OTEL Collector components provide default configs such that users do not need to provide most settings.
  • However, due to our organization of the storage extension config, there was no way to provide defaults

Description of the changes

  • 🛑 breaking Change the main config inside out: instead of static separation by storage types followed by a map of custom names, move the map to the top level followed by static separation by storage type. E.g.
# before
    memory:
      some_storage:
        max_traces: 100000
# after
    backends:
      some_storage:
        memory:
          max_traces: 100000
  • This required an introduction of an extra nesting via backends, which is a bit unfortunate, but the OTEL framework requires config to be a struct, it did not work with a map at the top level. Having a struct might actually be beneficial in the future if we need some top-level settings
  • Add custom marshaling code where if a specific storage type is detected as set-by-user, then a default is created first (similar mechanism to how extension default configs is created by OTEL Collector, but unfortunately that mechanism is rigid, non-recursive)
  • Change sample config files to the new format
  • Use names some_store and another_store in configs to emphasize that those are custom names, not part of the config struct
  • Add more unit tests for config

How was this change tested?

  • Unit tests and CI

Checklist

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.40%. Comparing base (9ba3238) to head (5a6a730).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5691      +/-   ##
==========================================
+ Coverage   96.38%   96.40%   +0.01%     
==========================================
  Files         334      334              
  Lines       16141    16144       +3     
==========================================
+ Hits        15558    15563       +5     
+ Misses        405      404       -1     
+ Partials      178      177       -1     
Flag Coverage Δ
badger_v1 8.06% <32.50%> (+0.01%) ⬆️
badger_v2 1.90% <0.00%> (-0.03%) ⬇️
cassandra-3.x-v1 16.63% <42.50%> (+0.03%) ⬆️
cassandra-3.x-v2 1.82% <0.00%> (-0.02%) ⬇️
cassandra-4.x-v1 16.63% <42.50%> (+0.03%) ⬆️
cassandra-4.x-v2 1.82% <0.00%> (-0.02%) ⬇️
elasticsearch-7.x-v1 18.86% <5.00%> (-0.01%) ⬇️
elasticsearch-8.x-v1 19.06% <5.00%> (-0.01%) ⬇️
elasticsearch-8.x-v2 19.06% <5.00%> (-0.03%) ⬇️
grpc_v1 9.45% <0.00%> (-0.03%) ⬇️
grpc_v2 7.39% <0.00%> (-0.09%) ⬇️
kafka 9.75% <0.00%> (-0.02%) ⬇️
opensearch-1.x-v1 18.90% <5.00%> (-0.03%) ⬇️
opensearch-2.x-v1 18.91% <5.00%> (-0.03%) ⬇️
opensearch-2.x-v2 18.91% <5.00%> (-0.03%) ⬇️
unittests 94.19% <90.81%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
if _, ok := s.ext.factories[name]; ok {
return fmt.Errorf("duplicate %s storage name %s", s.storageKind, name)
func (s *storageExt) Start(_ context.Context, _ component.Host) error {
mf := otelmetrics.NewFactory(s.telset.MeterProvider)
Copy link
Member Author

Choose a reason for hiding this comment

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

@Wise-Wizard using your new lib here!

Copy link
Contributor

Choose a reason for hiding this comment

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

🥳🥳🥳

Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro marked this pull request as ready for review July 1, 2024 02:10
@yurishkuro yurishkuro requested a review from a team as a code owner July 1, 2024 02:10
@dosubot dosubot bot added area/storage changelog:breaking-change Change that is breaking public APIs or established behavior storage/badger Issues related to badger storage storage/cassandra storage/elasticsearch labels Jul 1, 2024
Copy link
Contributor

@jkowall jkowall left a comment

Choose a reason for hiding this comment

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

LGTM!

@yurishkuro yurishkuro merged commit d6827e3 into jaegertracing:main Jul 1, 2024
49 checks passed
@yurishkuro yurishkuro deleted the config-with-defaults branch July 1, 2024 05:55
@yurishkuro yurishkuro added changelog:exprimental Change to an experimental part of the code and removed changelog:breaking-change Change that is breaking public APIs or established behavior labels Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage changelog:exprimental Change to an experimental part of the code storage/badger Issues related to badger storage storage/cassandra storage/elasticsearch v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants