Skip to content

feat: parse general attribute limits in OTelEnv#74

Open
harshitt13 wants to merge 3 commits into
MindfulSoftwareLLC:mainfrom
harshitt13:feature/issue-20-gen-attribute-limits
Open

feat: parse general attribute limits in OTelEnv#74
harshitt13 wants to merge 3 commits into
MindfulSoftwareLLC:mainfrom
harshitt13:feature/issue-20-gen-attribute-limits

Conversation

@harshitt13

@harshitt13 harshitt13 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Closes #73

Used Opus 4.6

Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
@harshitt13
harshitt13 requested a review from michaelbushe as a code owner July 13, 2026 10:29
Comment thread lib/src/environment/otel_env.dart Outdated
…upport

Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
/// - Warnings should be logged when limits are exceeded.
///
/// See: https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#attribute-limits
static AttributeLimitsConfig getAttributeLimits() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The parsing here is solid, but nothing in lib/ consumes it — the only callers of getAttributeLimits() are the tests. The spec behavior this enables (truncate over-length values, drop excess attributes, warn when limits are exceeded) isn't implemented, and the fallback chain the spec defines (signal-specific limits override the general ones; when OTEL_LOGRECORD_ATTRIBUTE_* is unset, OTEL_ATTRIBUTE_* applies) isn't wired into getLogRecordLimits() below either.

That's fine as a first slice! But then the PR should say so: please change "Closes #73" to "Part of #73" and note the follow-ups (enforcement at the attribute-creation/record boundaries, plus the general→signal fallback). Otherwise #73 auto-closes with the env vars parsed but having no observable effect, which is the kind of gap that surfaces as a confusing bug report later. The fallback wiring would also make a nice, self-contained next PR.

// Get attribute value length limit
final valueLengthLimit = _getEnv(otelAttributeValueLengthLimit);
if (valueLengthLimit != null) {
final limit = int.tryParse(valueLengthLimit);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

int.tryParse happily accepts -5, and a negative limit is meaningless — please reject non-negative violations, and warn (not debug, and not silence) when a set value is unusable, per the spec's guidance and consistent with where #66 landed. If you fold the parsing into a shared helper (see the config-class comment), the range check and the warn live in exactly one place for both the general and logrecord variants.

/// - Warnings should be logged when limits are exceeded.
///
/// See: https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#attribute-limits
class AttributeLimitsConfig {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the pattern, nicely done — typed config class, CNCF header, barrel export, all right on the first pass. One structural upgrade: getAttributeLimits() and getLogRecordLimits() are now line-for-line copies differing only in which env vars they read. A single parameterized factory here collapses both:

factory AttributeLimitsConfig.fromEnvironment({
  required String valueLengthVar,
  required String countVar,
}) { ... }

with OTelEnv (or callers) passing otelAttributeValueLengthLimit/otelLogrecordAttributeValueLengthLimit etc. Parsing, range validation, and the warn then exist exactly once, and the future span/event/link limit vars (OTEL_SPAN_ATTRIBUTE_* and friends, already in env_constants.dart) become one-liners.

///
/// Only includes fields that are non-null, matching the previous
/// `Map<String, dynamic>` behavior for backward compatibility.
Map<String, dynamic> toJson() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

toJson() has no production consumer — only the test helpers use it, and they can build their own maps. "Matching the previous Map behavior for backward compatibility" isn't needed here since the map shape was never a published API. Pre-1.0, the cheapest API surface to maintain is the one you didn't ship — suggest dropping it (or moving it into the test helper if the subprocess tests want JSON).

final config = <String, dynamic>{};
/// Returns an [AttributeLimitsConfig] containing the log record attribute
/// limits. Fields that are not set via environment variables will be `null`.
static AttributeLimitsConfig getLogRecordLimits() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rebase heads-up: since #84 merged, main has in-process tests (test/unit/environment/otel_env_inprocess_test.dart) that assert getLogRecordLimits() returns the map shape — changing the return type to AttributeLimitsConfig will break them until they're updated to the new fields (a mechanical fix, and honestly an improvement to those assertions). Main also now has EnvironmentService.testOverrides, which lets you test all of this in-process without the subprocess helpers — worth a look before adding more helper scripts.

And one more for the checklist: no CHANGELOG entry yet — please add one under ## [1.1.0-beta.10-wip] for the new OTEL_ATTRIBUTE_* parsing (noting enforcement is follow-up work).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Implement General Attribute Limits

3 participants