Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Span::setAttribute() doesn't accept null values #65

Closed
prydin opened this issue Jan 16, 2020 · 5 comments
Closed

Span::setAttribute() doesn't accept null values #65

prydin opened this issue Jan 16, 2020 · 5 comments

Comments

@prydin
Copy link
Contributor

prydin commented Jan 16, 2020

The Span::setAttribute() method is commonly used in interceptors to copy attributes from an invocation to span attributes. This method will, by design, throw a NullPointerException if a null value is supplied. Given that many frameworks may treat certain attributes as optional, there's a chance that attributes with null values are present on the invocation objects.

The current implementation instrumenters/decorators are wrapping the calls to setAttribute() in null-checking if-statements whenever it is suspected that a null value may be supplied.

However, since the exact rules for which attributes may contain null aren't well documented for all frameworks, there's a high risk that a null-check is omitted by mistake, causing hard to find errors at runtime.

Should we implement a setAttributeOrNull() method that can be used when the presence of null values isn't known? Should we change the behavior of setAttribute()? What is the reason we don't allow null values in the first place? Could we change the behavior to make an assignment of a null value to an attribute a no-op?

@trask
Copy link
Member

trask commented Jan 16, 2020

Can you open this in https://github.com/open-telemetry/opentelemetry-java? That is where this change would need to happen. (btw, I totally support this!)

@prydin
Copy link
Contributor Author

prydin commented Jan 16, 2020

OK. Will do. BTW, I'm seeing this issue in multiple packages. I think the original Datadog code was written without null-checking in mind. Leaving this check in place is a ticking bomb.

@prydin
Copy link
Contributor Author

prydin commented Jan 16, 2020

Linked issue: open-telemetry/opentelemetry-java#765

@trask
Copy link
Member

trask commented Jan 16, 2020

Yeah, the existing code does null-checking here:

https://github.com/open-telemetry/opentelemetry-auto-instr-java/blob/686f72e02f70657ba2b46e57276bfa60ce9a033f/java-agent/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/AgentTracerImpl.java#L149

This problem is coming up when moving the instrumentation to use OpenTelemetry API directly.

@trask
Copy link
Member

trask commented Jan 23, 2020

Closing (tracking subsequent cleanup in #76)

@trask trask closed this as completed Jan 23, 2020
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

No branches or pull requests

2 participants