[CONFIGURATION] Apply general attribute_limits to tracer and logger providers - #4468
[CONFIGURATION] Apply general attribute_limits to tracer and logger providers#4468ayush-singh-0601 wants to merge 7 commits into
Conversation
…roviders The YAML parser already accepted attribute_limits, but SdkBuilder ignored them. Pass the general limits into CreateTracerProvider and CreateLoggerProvider, and use them when the provider has no model-specific limits. Provider limits still take precedence. Fixes open-telemetry#4467
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4468 +/- ##
==========================================
+ Coverage 82.67% 82.72% +0.05%
==========================================
Files 516 517 +1
Lines 20197 20257 +60
==========================================
+ Hits 16696 16755 +59
- Misses 3501 3502 +1
🚀 New features to boost your workflow:
|
|
can you please any changes that i need to make or is it fine |
|
all checks are passed , please tell if any more changes needed |
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the fix! Requesting a minor change noted below.
|
@dbarker done |
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the updates. Diving deeper into the spec and use cases I'm requested a change to the limit models and application methods to meet the spec.
| } | ||
| else | ||
| { | ||
| ApplyGeneralAttributeLimits(span_limits, attribute_limits); |
There was a problem hiding this comment.
This approach does not fully meet the spec in that the AttributeLimits are applied at the YAML node limit group level (SpanLimits and LogRecordLimits) instead of per individual limit.
If both a general and a model-specific limit are implemented, then the SDK MUST first attempt to use the model-specific limit, if it isn’t set, then the SDK MUST attempt to use the general limit. If neither are defined, then the SDK MUST try to use the model-specific limit default value, followed by the global limit default value.
With the following example a user may set the top-level attribute_limits then want to override specific limit fields in the provider configs while leaving some limits to retain the default values. Note that the schema supports explicitly setting null for each limit.
attribute_limits:
attribute_value_length_limit: 4096
attribute_count_limit: null
logger_provider:
limits:
attribute_value_length_limit: 1024
attribute_count_limit: null # `logger_provider.limits` default expected
tracer_provider:
limits:
attribute_value_length_limit: null # `attribute_limits` value expected
attribute_count_limit: null # `tracer_provider.limits` default expected
event_count_limit: 64
link_count_limit: 64
event_attribute_count_limit: 8
link_attribute_count_limit: 8A user may reasonably expect logger_provider.limits and tracer_provider.limits from the above to resolve to:
logger_provider:
limits:
attribute_value_length_limit: 1024 # set from explicit `logger_provider.limits` value
attribute_count_limit: 128 # set from `logger_provider.limits` default
tracer_provider:
limits:
attribute_value_length_limit: 4096 # from explicit `attribute_limits` value
attribute_count_limit: 128 # from the `tracer_provider.limits` default
event_count_limit: 64
link_count_limit: 64
event_attribute_count_limit: 8
link_attribute_count_limit: 8The issue is that the c++ model for AttributeLimitsConfiguration, SpanLimitsConfiguration, and LogRecordLimitsConfiguration do not represent individual limits as optional. We need to change the limit types from native numeric values to a type that can represent set or null (unset). Then the limit merge algorithm can be implemented to meet the spec.
Please consider this and update the design to meet the spec. Breaking changes to the model classes are okay now since the configuration libraries are still marked experimental and just need to be communicated in the CHANGELOG.
Make each limit on AttributeLimits, SpanLimits, and LogRecordLimits optional so omitted keys and YAML null are unset. Merge uses the model-specific value if set, else the general attribute_limits value, else the model-specific default.
|
@dbarker updated. Limit fields are optional now so omitted/null is distinct from a set value. Merge is model-specific if set, else general attribute_limits, else the model default. |
|
Thanks for syncing main into the branch. I checked the latest run, and all eight GitHub Actions workflows are currently marked action_required, so they have not actually executed yet. Could you approve them when convenient? The previous run did show IWYU, formatting, and several C++14/CMake failures, so once the fresh run is available I will address any remaining PR-specific issues together. Thanks! |
2 participants
Fixes #4467
The YAML parser already accepted
attribute_limits, butSdkBuilderlogged a warning and ignored them. Per the spec, general attribute limits should apply when tracer/loggerlimitsare not set, and model-specific limits should win when they are.Changes
attribute_limitsfromCreateConfiguredSdkintoCreateTracerProviderandCreateLoggerProvider.limits, copyattribute_count_limitandattribute_value_length_limitfrom the general config intoSpanLimits/LogRecordLimits.limits, keep using those values (including span-only fields such as event/link limits).CreateTracerProviderandCreateLoggerProvidertake an optionalAttributeLimitsConfiguration*that defaults tonullptr.CreateConfiguredSdkwiring for both traces and logs.How changes were tested
SdkBuilder.SpanLimitsFromAttributeLimitsandSdkBuilder.SpanLimitsOverrideAttributeLimits.GetSpanLimits()on the configured tracer provider.I could not run the C++ test binaries in this environment (no local CMake/Bazel build of opentelemetry-cpp). CI should run
sdk_builder_testandprogrammatic_configuration_test.CHANGELOG.mdupdated for non-trivial changes