Skip to content

Metrics - Document null as an accepted attribute value (Attribute::tryFromValue() already supports it)Β #2182

Description

@JulienChavee

Problem Statement

This is a documentation/type-hint improvement rather than a bug: null attribute values already work at runtime, they are simply not declared as valid, so static analysis rejects what the SDK happily accepts.

In sentry/sentry 4.30.0 attribute values are declared as string|bool|int|float:

// src/Attributes/Attribute.php
/**
 * @phpstan-type AttributeType 'string'|'boolean'|'integer'|'double'
 * @phpstan-type AttributeValue string|bool|int|float
 */

and the public entry points that take attributes carry the same narrowed type:

// src/Metrics/TraceMetrics.php
/**
 * @param int|float                            $value
 * @param array<string, int|float|string|bool> $attributes
 */
public function count(string $name, $value, array $attributes = [], ?Unit $unit = null): void

But null is explicitly handled, and handled deliberately:

// src/Attributes/Attribute.php::tryFromValue()
if ($value === null) {
    return new self('null', 'string');
}

So this call is fully functional, yet unrepresentable in the declared types:

\Sentry\metrics()->count('orders.created', 1, ['tenant' => null]);
// works; arrives in Sentry as tenant="null" (type string)

Impact for consumers: wrappers around the SDK that want to accept array<string, int|float|string|bool|null> cannot pass it through. On PHPStan level 9 this produces

Parameter #3 $attributes of method Sentry\Metrics\TraceMetrics::count() expects
array<string, bool|float|int|string>, array<string, bool|float|int|string|null> given.

Note also that the SDK already has an explicit "skip null" path (AttributeBag::setUnlessNull()), which suggests null reaching set()/tryFromValue() is expected and intentionally given the "null" string representation rather than being an oversight.

Solution Brainstorm

  1. declare what already happens. Add null to the attribute @param docblocks (TraceMetrics::count()/gauge()/distribution(), log attributes). No behaviour change, no breaking change, and consumers can drop their suppressions.

  2. Document without widening the type. Keep the types as-is but state in the docblock/docs that null is tolerated and stringified. But static analysis still blocks pass-through, so wrappers keep their ignores.

  3. If the "null" string is not the intended representation, then the docs are right and the implementation is the odd one out

Happy to open a PR for whichever direction the maintainers prefer.

Metadata

Metadata

Assignees

No one assigned

    Projects

    Status
    Waiting for: Product Owner

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions