feat: implement BatchLogRecordProcessor environment variables and val…

[harshitt13]

Closes: #65

[michaelbushe]

Nice work, please improve according to the comments. Consistency, simplicity, DRYness and Single Responsibility are always great goals.

[robert-northmind]

Hey, looks very good. Thanks for the contribution
I just left some comments. Let me know what you think. I am also new to this codebase. so i might not be right in all my thoughts ☺️

[robert-northmind]

One thought here. I know there is an existing pattern here to return Map<String, dynamic> objects. But what if we instead returned like a named dart record. Something like:

typedef BlrpEnvironmentValues = ({
  Duration? scheduleDelay,
  Duration? exportTimeout,
  int? maxQueueSize,
  int? maxExportBatchSize,
});

Then we get the benefit that we know the types of each entry. and we don't need to use string-keys to find them. I think it would in general make the code a bit safer.
It reduces the need for like as int? or is Duration?
Not sure if we wanna go down this path though.
What are your thoughts on this @michaelbushe ?

[harshitt13]

Nice idea, but I'll only change BLRP in this PR to keep scope manageable. We could migrate the others (especially BSP, since it's structurally identical) in a follow-up PR if you think the pattern works well. Thoughts @michaelbushe?

[robert-northmind]

One thought here. Does this pub a bit of Blrp domain knowledge in the OTelEnv parsing? Like that a queue needs to be bigger than 0?
What if the OTelEnv parsing only checks if the env variable exisits and has right type? Like it could warn if an int is a string.

But then the logic to check if the acutal int is bigger than 0, that could happen in the BatchLogRecordProcessorConfig.fromEnvironment()?

[harshitt13]

Good catch, you're right. I'll update getBlrpConfig() to only do type-checking (parse the int, warn if non-numeric) and move the > 0 validation into fromEnvironment().

[robert-northmind]

We have now this 2048 defined here and in the constructor in line 46 also, and in a few other places in this file.
should we break that our to a private static default variable?
Like: static const _defaultQueueSize = 2048;

[harshitt13]

I'd make them public rather than private (defaultMaxQueueSize instead of _defaultMaxQueueSize), since tests and external consumers might want to reference the spec defaults. Does that work for you?

[robert-northmind]

What if we make the BatchLogRecordProcessorConfig constructor private, and make all its params required. So that we dont have to provide the defaults there.
And instead, we can only create BatchLogRecordProcessorConfig either via the factory BatchLogRecordProcessorConfig.fromEnvironment() or a factory BatchLogRecordProcessorConfig({...});.

Then in those factories we could do the validation. something like:

factory BatchLogRecordProcessorConfig({
    int maxQueueSize = defaultQueueSize,
    // ...
  }) {
    if (!_validQueueSize(maxQueueSize)) {
      throw ArgumentError.value(maxQueueSize, 'maxQueueSize');
    }
    return BatchLogRecordProcessorConfig._(
      maxQueueSize: maxQueueSize,
    );
  }

factory BatchLogRecordProcessorConfig.fromEnvironment() {
    final env = OTelEnv.getBlrpConfig();
    final queueSize = _validQueueSize(env.maxQueueSize)
        ? env.maxQueueSize!
        : defaultQueueSize;
    if (env.maxQueueSize != null && !_validQueueSize(env.maxQueueSize)) {
      OTelLog.warn(/* invalid environment value */);
    }
    return BatchLogRecordProcessorConfig(
      maxQueueSize: queueSize,
    );
  }

[harshitt13]

The idea of centralizing validation is good, but making the constructor a factory would break const construction; the codebase currently uses const BatchLogRecordProcessorConfig() in several places (including tests and the BatchLogRecordProcessor default). Factory constructors can't be const in Dart.

Also, the existing BatchSpanProcessorConfig (BSP) uses the same pattern (public const constructor + fromEnvironment() factory) without private constructors, so changing BLRP alone would create an inconsistency between the two.

Options:

1. Go with your pattern and accept the const break (minor, just tests/defaults need updating) for cleaner validation
2. Or keep the public constructor, extract defaults to static const, and do validation only in fromEnvironment() (consistent with BSP)

What do you prefer?

cc: @michaelbushe

[robert-northmind]

does this need to be private? if it would be public, we could easily directly test the logic in here. Just a thought

[harshitt13]

It's already indirectly tested through getBlrpConfig() and getBspConfig(); the in-process tests and subprocess tests cover the non-numeric, below-minimum, and above-maximum branches.
It is a genuinely reusable parsing utility, so making it public isn't harmful. I could go either way:

• Make it public (it's a useful utility on a public class)
• Keep it private with @visibleForTesting
• Leave it private since it's already well-covered

What's your preference?

cc: @michaelbushe

[robert-northmind]

I didn't quite follow the need for this one. this is only used for testing right?
is there some other way to structure this. so that we dont need public api for testing only needs?

[harshitt13]

Right, I'll remove it and update the test to validate clamping behavior by constructing BatchLogRecordProcessorConfig(maxQueueSize: 100, maxExportBatchSize: 200) directly.