Provide guidance for additional null check

[jack-berg]

We need to get consistent about whether or not we do this. We rely on javax.annotation.Nullable annotation, the assumption that params and returns are non-null unless otherwise specified, leveraging the net.ltgt.nullaway error prone plugin to validate at build time. Of course, at runtime there is no guarantee that callers adhere to these annotations, and so we have code like this which checks for null despite the params being non-null according to the annotations.

We need to be consistent about:

• When and why do we add additional null checks
• Where do we do this (i.e. in interface default methods or in the actual implementation)
• What is the behavior when the null annotation is violated (throw or noop)

Let's hold off here until we get some repo level guidance on this. ConfigProperties, the env var / sys property analog of DeclarativeConfigProperties, has the exact same method and does not do this null check.

Originally posted by @jack-berg in #8079

[Tusharika725]

Hi @jack-berg! I am an aspiring contributor and I would love to help add this guidance to the documentation as my first issue.

Since this requires a consensus on the project's design philosophy, has the maintainer team reached a decision on the three points you mentioned (When/Why, Where, and Behavior)?

Once there is a decided direction, I would be happy to draft the updates for CONTRIBUTING.md (or wherever you prefer this guidance to live). Let me know how I can help move this discussion forward!

[jack-berg]

Since this requires a consensus on the project's design philosophy, has the maintainer team reached a decision on the three points you mentioned (When/Why, Where, and Behavior)?

No consensus yet. Consensus would likely look like a proposal on this issue with upvotes from approvers / maintainers.

My proposal:

• Add checks only at API boundaries. I.e. once we're within the walled garden of the implemetnation, trust nullaway.
• Different behavior for instrumentation API vs. SDK API boundaries:
  • Since SDK API is largely (all?) programmatic config surface area, adopt fail fast philosophy. I.e. check for nullness and throw a NPE if a non-null argument is null.
  • Since the instrumentation API is called continuously throughout application lifecycle, do not throw exceptions when null constraints are violated. Instead, gracefully degrade by implementing with noop, or returning noop API when needed. Open question of whether we should silently degrade or log (maybe with throttle?). I propose silently degrading to noop.
• Propose implementing these checks only in concrete implementations, or in default interface implementations if they are added later and are exposed to NPEs without checks. I do not want us to go back and retroactively add default implementations to all interface methods just for null safety.

[Tusharika725]

@jack-berg Thanks for outlining this so clearly! The distinction between failing fast at the SDK boundary (configuration/startup time) and silently degrading to noop at the Instrumentation API boundary (live runtime) makes complete architectural sense we definitely don't want telemetry exceptions crashing a user's live application.

To help drive this consensus forward, would it be helpful if I opened a Draft PR adding your exact proposal to CONTRIBUTING.md (or a dedicated architecture document)?

That would give the other approvers a concrete piece of documentation to review, tweak, and approve. Once consensus is reached and the docs are merged, I'd also be happy to help implement these boundary checks on a few classes to get the ball rolling. Let me know if you'd like me to open that Draft PR!